Re: [PATCH] scsi: lpfc: Fix crash on PCI hotplug remove path

2017-05-29 Thread James Smart 

looks good

Signed-off-by: James Smart  

-- james



On 5/28/2017 2:45 PM, Guilherme G. Piccoli wrote:

During a PCI hotplug remove event we could have a NULL pointer
dereference on lpfc_sli_abort_iocb(), if pring is NULL. This
patch adds a check for this case and is able to circumvent the
failure and continue the hotplug remove process with success.

This issue was introduced after the driver refactor made on
commit 895427bd012c ("scsi: lpfc: NVME Initiator: Base modifications").

Fixes: 895427bd012c ("scsi: lpfc: NVME Initiator: Base modifications")
Reported-by: Naresh Bannoth 
Signed-off-by: Guilherme G. Piccoli 
---
This patch was rebased against Martin's 4.12/scsi-fixes.

Re: [PATCH] scsi: lpfc: fix spelling mistake "entrys" -> "entries"

2017-05-29 Thread James Smart 

it's good :)

Signed-off-by: James Smart 

-- james


On 5/26/2017 3:11 AM, Colin King wrote:

From: Colin Ian King 

Trivial fix to spelling mistake in debugfs message

Signed-off-by: Colin Ian King

Re: [PATCH] scsi: lpfc: make a couple of functions static

2017-05-29 Thread James Smart 

Patch is fine.

Signed-off-by: James Smart  

-- james


On 5/18/2017 2:35 AM, Colin King wrote:

From: Colin Ian King 

functions lpfc_nvmet_cleanup_io_context and lpfc_nvmet_setup_io_context
can be made static as they do not need to be in global scope.

Cleans up sparse warnings:
   "warning: symbol 'lpfc_nvmet_cleanup_io_context' was not declared.
Should it be static?"
   "warning: symbol 'lpfc_nvmet_setup_io_context' was not declared.
Should it be static?"

Signed-off-by: Colin Ian King

Re: [PATCH] scsi: lpfc: Avoid NULL pointer dereference in lpfc_els_abort()

2017-05-29 Thread James Smart 

Change looks good

Signed-off-by: James Smart  

-- james


On 5/24/2017 2:48 PM, Guilherme G. Piccoli wrote:

We might have a NULL pring in lpfc_els_abort(), for example on
error recovery path, since queues are destroyed during error
recovery mechanism.

In this case, we should just drop the abort since the queues will
be recreated anyway. This patch just verifies for NULL pointer
and stop the abortion of the queue in case of a NULL pring.

Also, this patch converts return type of lpfc_els_abort() from int
to void, since it's not checked anywhere.

Reported-by: Harsha Thyagaraja 
Reported-by: Naresh Bannoth 
Tested-by: Raphael Silva 
Signed-off-by: Guilherme G. Piccoli

Re: [PATCH] scsi: lpfc: prevent potential null pointer dereference

2017-05-29 Thread James Smart 

Looks good

Signed-off-by: James Smart 

-- james


On 5/23/2017 8:09 AM, Gustavo A. R. Silva wrote:

Null check at line 966: if (ndlp) {,  implies that ndlp might be NULL.
Functions lpfc_nlp_set_state() and lpfc_issue_els_prli() dereference
pointer ndlp. Include these function calls inside the IF block that
tests pointer ndlp.

Addresses-Coverity-ID: 1401856

Re: [PATCH] lpfc: nvmet_fc: fix format string

2017-05-29 Thread James Smart 

Patch is fine.

Signed-off-by: James Smart 

-- james


On 5/19/2017 1:04 AM, Arnd Bergmann wrote:

The lpfc_nvmeio_data() tracing helper always takes a format string and
three additional arguments. The latest caller has a format string with
only two integer arguments, causing this harmless warning:

drivers/scsi/lpfc/lpfc_nvmet.c: In function 'lpfc_nvmet_xmt_fcp_release':
drivers/scsi/lpfc/lpfc_nvmet.c:802:25: error: too many arguments for format 
[-Werror=format-extra-args]
   lpfc_nvmeio_data(phba, "NVMET FCP FREE: xri x%x ste %d\n", ctxp->oxid,

We could add a dummy argument here, but it seems reasonable to print
the 'abort' flag as the third argument.

Fixes: 19b58d9473e8 ("nvmet_fc: add req_release to lldd api")
Signed-off-by: Arnd Bergmann 
---

Re: [PATCH] scsi: lpfc: Fix crash on PCI hotplug remove path

2017-05-29 Thread Raphael Philipe Mendes da Silva 
On Mon, May 29, 2017 at 09:56:09AM +0200, Johannes Thumshirn wrote:
> On 05/28/2017 11:45 PM, Guilherme G. Piccoli wrote:
> > During a PCI hotplug remove event we could have a NULL pointer
> > dereference on lpfc_sli_abort_iocb(), if pring is NULL. This
> > patch adds a check for this case and is able to circumvent the
> > failure and continue the hotplug remove process with success.
> > 
> > This issue was introduced after the driver refactor made on
> > commit 895427bd012c ("scsi: lpfc: NVME Initiator: Base modifications").
> > 
> > Fixes: 895427bd012c ("scsi: lpfc: NVME Initiator: Base modifications")
> > Reported-by: Naresh Bannoth 
> > Signed-off-by: Guilherme G. Piccoli 
> > ---
> 
> Looks good,
> Reviewed-by: Johannes Thumshirn 

Tested-by: Raphael Silva

Re: [PATCH v1 3/4] tcmu: Make dev_size configurable via userspace

2017-05-29 Thread Mike Christie 
On 05/26/2017 09:27 AM, Bryant G. Ly wrote:
> Allow tcmu backstores to be able to set the device size
> after it has been configured via set attribute.
> 
> Part of support in userspace to support certain backstores
> changing device size.
> 
> Signed-off-by: Bryant G. Ly 
> ---
>  drivers/target/target_core_user.c | 59 
> +++
>  1 file changed, 54 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/target/target_core_user.c 
> b/drivers/target/target_core_user.c
> index ae91822..c8c84b7 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -1548,6 +1548,44 @@ static ssize_t tcmu_cmd_time_out_store(struct 
> config_item *item, const char *pag
>  }
>  CONFIGFS_ATTR(tcmu_, cmd_time_out);
>  
> +static ssize_t tcmu_dev_size_show(struct config_item *item, char *page)
> +{
> + struct se_dev_attrib *da = container_of(to_config_group(item),
> + struct se_dev_attrib, da_group);
> + struct tcmu_dev *udev = TCMU_DEV(da->da_dev);
> +
> + return snprintf(page, PAGE_SIZE, "%zu\n", udev->dev_size);
> +}
> +
> +static ssize_t tcmu_dev_size_store(struct config_item *item, const char 
> *page,
> +size_t count)
> +{
> + struct se_dev_attrib *da = container_of(to_config_group(item),
> + struct se_dev_attrib, da_group);
> + struct tcmu_dev *udev = TCMU_DEV(da->da_dev);
> + unsigned long val;
> + int ret;
> +
> + ret = kstrtoul(page, 0, );
> + if (ret < 0)
> + return ret;
> + udev->dev_size = val;
> +
> + /* Check if device has been configured before */
> + if (tcmu_dev_configured(udev)) {
> + ret = tcmu_netlink_event(TCMU_CMD_RECONFIG_DEVICE,


If we send an event for each attribute changed, maybe we want to add a
type or more info about the attr being changed, so userspace can easily
figure it out and it does not have to rescan everything multiple times.

Or maybe we just want to add a reconfig attr or control opt where after
userspace has reconfigured everything it wanted to runner can rescan
everything and updates itself.

Re: [PATCH v1 4/4] tcmu: Make dev_config configurable

2017-05-29 Thread Mike Christie 
On 05/26/2017 09:27 AM, Bryant G. Ly wrote:
> This allows for userspace to change the device path after
> it has been created. Thus giving the user the ability to change
> the path. The use case for this is to allow for virtual optical
> to have media change.
> 
> Signed-off-by: Bryant G. Ly 
> ---
>  drivers/target/target_core_user.c | 40 
> +++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/drivers/target/target_core_user.c 
> b/drivers/target/target_core_user.c
> index c8c84b7..3036a57 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -1548,6 +1548,45 @@ static ssize_t tcmu_cmd_time_out_store(struct 
> config_item *item, const char *pag
>  }
>  CONFIGFS_ATTR(tcmu_, cmd_time_out);
>  
> +static ssize_t tcmu_dev_path_show(struct config_item *item, char *page)
> +{
> + struct se_dev_attrib *da = container_of(to_config_group(item),
> + struct se_dev_attrib, da_group);
> + struct tcmu_dev *udev = TCMU_DEV(da->da_dev);
> +
> + return snprintf(page, PAGE_SIZE, "%s\n", udev->dev_config);
> +}
> +
> +static ssize_t tcmu_dev_path_store(struct config_item *item, const char 
> *page,
> +size_t count)
> +{
> + struct se_dev_attrib *da = container_of(to_config_group(item),
> + struct se_dev_attrib, da_group);
> + struct tcmu_dev *udev = TCMU_DEV(da->da_dev);
> + char *copy = NULL;
> +
> + copy = kstrdup(page, GFP_KERNEL);

Cen remove the extra newline

> +
> + if (!copy)
> + return -EINVAL;
> +
> + strcpy(udev->dev_config, copy);


Missing a kree(copy);


> +
> + /* Check if device has been configured before */
> + if (tcmu_dev_configured(udev)) {
> + ret = tcmu_netlink_event(TCMU_CMD_RECONFIG_DEVICE,
> +  udev->uio_info.name,
> +  udev->uio_info.uio_dev->minor);
> + if (ret) {
> + pr_err("Unable to reconfigure device\n");
> + return ret;
> + }
> + }
> +
> + return count;
> +}
> +CONFIGFS_ATTR(tcmu_, dev_path);
> +
>  static ssize_t tcmu_dev_size_show(struct config_item *item, char *page)
>  {
>   struct se_dev_attrib *da = container_of(to_config_group(item),
> @@ -1626,6 +1665,7 @@ CONFIGFS_ATTR(tcmu_, emulate_write_cache);
>  
>  struct configfs_attribute *tcmu_attrib_attrs[] = {
>   _attr_cmd_time_out,
> + _attr_dev_path,
>   _attr_dev_size,
>   _attr_emulate_write_cache,
>   NULL,
>

Re: [PATCH v2 07/22] scsi: hisi_sas: create hisi_sas_get_fw_info()

2017-05-29 Thread Arnd Bergmann 
On Mon, May 29, 2017 at 1:18 PM, John Garry  wrote:
> On 29/05/2017 11:53, Arnd Bergmann wrote:
>> On Thu, May 25, 2017 at 2:04 PM, John Garry  wrote:
>
> So we only require these properties for platform device with DT firmware.
> This code is same as before (apart from adding the comments), but I'll
> consider adding a verbose comment.
>
> As for the check, effectively I already have what you recommend in how np is
> evaluated:
> +   struct platform_device *pdev = hisi_hba->platform_dev;
> +   struct device_node *np = pdev ? pdev->dev.of_node : NULL;
> struct clk *refclk;

Ah, very good. Thanks for checking.

   Arnd

Re: [PATCH v2 07/22] scsi: hisi_sas: create hisi_sas_get_fw_info()

2017-05-29 Thread John Garry 

 is
On 29/05/2017 11:53, Arnd Bergmann wrote:

On Thu, May 25, 2017 at 2:04 PM, John Garry  wrote:

Move the functionality to retrieve the fw info into
a dedicated device type-agnostic function,
hisi_sas_get_fw_info().

The reasoning is that this function will be required
for future pci-based platforms.




-
if (device_property_read_u8_array(dev, "sas-addr", hisi_hba->sas_addr,
- SAS_ADDR_SIZE))
-   goto err_out;
+ SAS_ADDR_SIZE)) {
+   dev_err(dev, "could not get property sas-addr\n");
+   return -ENOENT;
+   }

if (np) {
hisi_hba->ctrl = syscon_regmap_lookup_by_phandle(np,
"hisilicon,sas-syscon");
-   if (IS_ERR(hisi_hba->ctrl))
-   goto err_out;
+   if (IS_ERR(hisi_hba->ctrl)) {
+   dev_err(dev, "could not get syscon\n");
+   return -ENOENT;
+   }


If I read this right, it will fail to work for a PCI-based driver trying to read
"sas-addr" but not the other properties that would now be hardcoded from
the PCI ID.

Maybe you just need


Hi Arnd,

So we only require these properties for platform device with DT 
firmware. This code is same as before (apart from adding the comments), 
but I'll consider adding a verbose comment.


As for the check, effectively I already have what you recommend in how 
np is evaluated:

+   struct platform_device *pdev = hisi_hba->platform_dev;
+   struct device_node *np = pdev ? pdev->dev.of_node : NULL;
struct clk *refclk;

Much appreciated,
John





-  if (np) {
+ if (np && is_platform_device) {

   Arnd

.

Re: [PATCH v2 07/22] scsi: hisi_sas: create hisi_sas_get_fw_info()

2017-05-29 Thread Arnd Bergmann 
On Thu, May 25, 2017 at 2:04 PM, John Garry  wrote:
> Move the functionality to retrieve the fw info into
> a dedicated device type-agnostic function,
> hisi_sas_get_fw_info().
>
> The reasoning is that this function will be required
> for future pci-based platforms.
>

> -
> if (device_property_read_u8_array(dev, "sas-addr", hisi_hba->sas_addr,
> - SAS_ADDR_SIZE))
> -   goto err_out;
> + SAS_ADDR_SIZE)) {
> +   dev_err(dev, "could not get property sas-addr\n");
> +   return -ENOENT;
> +   }
>
> if (np) {
> hisi_hba->ctrl = syscon_regmap_lookup_by_phandle(np,
> "hisilicon,sas-syscon");
> -   if (IS_ERR(hisi_hba->ctrl))
> -   goto err_out;
> +   if (IS_ERR(hisi_hba->ctrl)) {
> +   dev_err(dev, "could not get syscon\n");
> +   return -ENOENT;
> +   }

If I read this right, it will fail to work for a PCI-based driver trying to read
"sas-addr" but not the other properties that would now be hardcoded from
the PCI ID.

Maybe you just need

-  if (np) {
+ if (np && is_platform_device) {

   Arnd

Re: Charitable Organization~

2017-05-29 Thread .Sarah Edward J 
`._..

Glad to write to you this message,

I seek for your kind help in setting up a charitable organization to
help the less privileged  people and also the elderly people under
your care,

I want to use my late husband wealth of $3,000,000.00 to set up a
charity foundation to help the needy and the less privileged ones in
your country under your care, Can you help to build this project in
your country?

All i need from you is your sincerity to use this funds to do this
project as i desired to use it for less privileged ones and orphanage
homes,

We are the one who will make the world a better place when we help the
needy sincerely,

Reply and let me know your opinion in doing this noble work,

Remain blessed,

Mrs Sarah Edward J.

Re: [PATCH] scsi: lpfc: Avoid NULL pointer dereference in lpfc_els_abort()

2017-05-29 Thread Johannes Thumshirn 
On 05/24/2017 11:48 PM, Guilherme G. Piccoli wrote:
> We might have a NULL pring in lpfc_els_abort(), for example on
> error recovery path, since queues are destroyed during error
> recovery mechanism.
> 
> In this case, we should just drop the abort since the queues will
> be recreated anyway. This patch just verifies for NULL pointer
> and stop the abortion of the queue in case of a NULL pring.
> 
> Also, this patch converts return type of lpfc_els_abort() from int
> to void, since it's not checked anywhere.
> 
> Reported-by: Harsha Thyagaraja 
> Reported-by: Naresh Bannoth 
> Tested-by: Raphael Silva 
> Signed-off-by: Guilherme G. Piccoli 
> ---

Looks good,
Reviewed-by: Johannes Thumshirn 


-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH] scsi: lpfc: Fix crash on PCI hotplug remove path

2017-05-29 Thread Johannes Thumshirn 
On 05/28/2017 11:45 PM, Guilherme G. Piccoli wrote:
> During a PCI hotplug remove event we could have a NULL pointer
> dereference on lpfc_sli_abort_iocb(), if pring is NULL. This
> patch adds a check for this case and is able to circumvent the
> failure and continue the hotplug remove process with success.
> 
> This issue was introduced after the driver refactor made on
> commit 895427bd012c ("scsi: lpfc: NVME Initiator: Base modifications").
> 
> Fixes: 895427bd012c ("scsi: lpfc: NVME Initiator: Base modifications")
> Reported-by: Naresh Bannoth 
> Signed-off-by: Guilherme G. Piccoli 
> ---

Looks good,
Reviewed-by: Johannes Thumshirn 

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850