Re: [PATCH] scsi_dh_alua: Declare local functions static

[Hannes Reinecke]

On 04/14/2016 07:27 PM, Bart Van Assche wrote:
> This patch avoids that building with W=1 causes gcc to report the
> following type of warning:
> 
> no previous prototype for ... [-Wmissing-prototypes]
> 
> Signed-off-by: Bart Van Assche 
> Cc: Hannes Reinecke 
> Cc: Christoph Hellwig 
> Cc: Ewan Milne 
> ---
>  drivers/scsi/device_handler/scsi_dh_alua.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c 
> b/drivers/scsi/device_handler/scsi_dh_alua.c
> index 8eaed05..e034f12 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -190,8 +190,8 @@ static int submit_stpg(struct scsi_device *sdev, int 
> group_id,
> ALUA_FAILOVER_RETRIES, NULL, req_flags);
>  }
>  
> -struct alua_port_group *alua_find_get_pg(char *id_str, size_t id_size,
> -  int group_id)
> +static struct alua_port_group *alua_find_get_pg(char *id_str, size_t id_size,
> + int group_id)
>  {
>   struct alua_port_group *pg;
>  
> @@ -219,8 +219,8 @@ struct alua_port_group *alua_find_get_pg(char *id_str, 
> size_t id_size,
>   * Allocate a new port_group structure for a given
>   * device.
>   */
> -struct alua_port_group *alua_alloc_pg(struct scsi_device *sdev,
> -   int group_id, int tpgs)
> +static struct alua_port_group *alua_alloc_pg(struct scsi_device *sdev,
> +  int group_id, int tpgs)
>  {
>   struct alua_port_group *pg, *tmp_pg;
>  
> 
Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [-next] BUG_ON in scsi_target_destroy()

[Xiong Zhou]

Hi,

On Wed, Apr 13, 2016 at 11:14 PM, James Bottomley
 wrote:
> On Wed, 2016-04-13 at 10:41 +0200, Johannes Thumshirn wrote:
>> Hi Sergey,  Xiong,
>>
>> Can you try below patch?
>>
>> On Montag, 11. April 2016 18:01:47 CEST Sergey Senozhatsky wrote:
>> > Hello,
>> >
>> > commit 7b106f2de6938c31ce5e9c86bc70ad3904666b96
>> > Author: Johannes Thumshirn 
>> > Date:   Tue Apr 5 11:50:44 2016 +0200
>> >
>> > scsi: Add intermediate STARGET_REMOVE state to
>> > scsi_target_state
>> >
>> >
>> > BUG_ON()s (next-20160411) each time I remove a usb flash
>> >
>> > [   49.561600]  [] scsi_target_destroy+0x5a/0xcb
>> > [scsi_mod]
>> > [   49.561607]  [] scsi_target_reap+0x4a/0x4f
>> > [scsi_mod]
>> > [   49.561613]  [] __scsi_remove_device+0xc3/0xd0
>> > [scsi_mod]
>> > [   49.561619]  [] scsi_forget_host+0x52/0x63
>> > [scsi_mod]
>> > [   49.561623]  [] scsi_remove_host+0x8c/0x102
>> > [scsi_mod]
>> > [   49.561627]  [] usb_stor_disconnect+0x6b/0xab
>> > [usb_storage]
>> > [   49.561634]  []
>> > usb_unbind_interface+0x77/0x1ca [usbcore]
>> > [   49.561636]  []
>> > __device_release_driver+0x9d/0x121
>> > [   49.561638]  []
>> > device_release_driver+0x23/0x30
>> > [   49.561639]  [] bus_remove_device+0xfb/0x10e
>> > [   49.561641]  [] device_del+0x164/0x1e6
>> > [   49.561648]  [] ?
>> > remove_intf_ep_devs+0x3b/0x48 [usbcore]
>> > [   49.561655]  [] usb_disable_device+0x84/0x1a5
>> > [usbcore]
>> > [   49.561661]  [] usb_disconnect+0x94/0x19f
>> > [usbcore]
>> > [   49.561667]  [] hub_event+0x5c1/0xdea
>> > [usbcore]
>> > [   49.561670]  [] process_one_work+0x1dc/0x37f
>> > [   49.561672]  [] worker_thread+0x282/0x36d
>> > [   49.561673]  [] ? rescuer_thread+0x2ae/0x2ae
>> > [   49.561675]  [] kthread+0xd2/0xda
>> > [   49.561678]  [] ret_from_fork+0x22/0x40
>> > [   49.561679]  [] ?
>> > kthread_worker_fn+0x13e/0x13e
>> >
>> > -ss
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux
>> > -scsi" in
>> > the body of a message to majord...@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >
>>
>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
>> index 0734927..0c00928 100644
>> --- a/drivers/scsi/scsi_sysfs.c
>> +++ b/drivers/scsi/scsi_sysfs.c
>> @@ -1276,6 +1276,7 @@ int scsi_sysfs_add_sdev(struct scsi_device
>> *sdev)
>>  void __scsi_remove_device(struct scsi_device *sdev)
>>  {
>>   struct device *dev = >sdev_gendev;
>> + struct scsi_target *starget;
>>
>>   /*
>>* This cleanup path is not reentrant and while it is
>> impossible
>> @@ -1315,7 +1316,9 @@ void __scsi_remove_device(struct scsi_device
>> *sdev)
>>* remoed sysfs visibility from the device, so make the
>> target
>>* invisible if this was the last device underneath it.
>>*/
>> - scsi_target_reap(scsi_target(sdev));
>> + starget = scsi_target(sdev);
>> + starget->state = STARGET_REMOVE;
>> + scsi_target_reap(starget);
>
> How about good grief no!  A device with multiple targets will get it's
> lists screwed with this
>
> The STARGET_REMOVE state you added only applies to the case we're
> trying to kill a target.  In the natural operation case, which is what
> everyone else is running into, we will try to remove a running target
> when it has no more scsi devices left on it.  So the correct patch
> should be to make the BUG_ON see this:
>
> James
>
> ---
>
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 27df7e7..e0a78f5 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -319,8 +319,7 @@ static void scsi_target_destroy(struct scsi_target 
> *starget)
> struct Scsi_Host *shost = dev_to_shost(dev->parent);
> unsigned long flags;
>
> -   BUG_ON(starget->state != STARGET_REMOVE &&
> -  starget->state != STARGET_CREATED);
> +   BUG_ON(starget->state == STARGET_DEL);
> starget->state = STARGET_DEL;
> transport_destroy_device(dev);
> spin_lock_irqsave(shost->host_lock, flags);
>


This will survive modprobe -r scsi_debug ..

Just to make sure, is this _REMOVE state able to do the latter thing what it was
trying to do, in this way ?


commit 7b106f2de6938c31ce5e9c86bc70ad3904666b96
Author: Johannes Thumshirn 
Date:   Tue Apr 5 11:50:44 2016 +0200

scsi: Add intermediate STARGET_REMOVE state to scsi_target_state

Add intermediate STARGET_REMOVE state to scsi_target_state to avoid
running into the BUG_ON() in scsi_target_reap(). The STARGET_REMOVE
state is only valid in the path from scsi_remove_target() to
scsi_target_destroy() indicating this target is going to be removed.

This re-fixes the problem introduced in commits bc3f02a795d3 ("[SCSI]
scsi_remove_target: fix softlockup regression on hot remove") and
40998193560d ("scsi: restart list search after unlock in
scsi_remove_target") in a more 

Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)

[Ingo Molnar]

* Denys Vlasenko  wrote:

> > In fact, the following patch seems to fix it:
> > 
> > diff --git a/include/scsi/scsi_transport_fc.h 
> > b/include/scsi/scsi_transport_fc.h
> > index bf66ea6..56b9e81 100644
> > --- a/include/scsi/scsi_transport_fc.h
> > +++ b/include/scsi/scsi_transport_fc.h
> > @@ -796,7 +796,7 @@ fc_remote_port_chkready(struct fc_rport *rport)
> > return result;
> >  }
> >  
> > -static inline u64 wwn_to_u64(u8 *wwn)
> > +static __always_inline u64 wwn_to_u64(u8 *wwn)
> >  {
> > return get_unaligned_be64(wwn);
> >  }
> 
> It is not a guarantee.

Of course it's a workaround - but is there any deterministic way to turn off 
this 
GCC bug (by activating some GCC command line switch), or do we have to live 
with 
objtool warning about this GCC?

Which, by the way, is pretty cool!

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [-next] BUG_ON in scsi_target_destroy()

[Xiong Zhou]

Hi,

On Wed, Apr 13, 2016 at 4:41 PM, Johannes Thumshirn  wrote:
> Hi Sergey,  Xiong,
>
> Can you try below patch?

This survives modprobe -r scsi_debug.

>
> On Montag, 11. April 2016 18:01:47 CEST Sergey Senozhatsky wrote:
>> Hello,
>>
>> commit 7b106f2de6938c31ce5e9c86bc70ad3904666b96
>> Author: Johannes Thumshirn 
>> Date:   Tue Apr 5 11:50:44 2016 +0200
>>
>> scsi: Add intermediate STARGET_REMOVE state to scsi_target_state
>>
>>
>> BUG_ON()s (next-20160411) each time I remove a usb flash
>>
>> [   49.561600]  [] scsi_target_destroy+0x5a/0xcb [scsi_mod]
>> [   49.561607]  [] scsi_target_reap+0x4a/0x4f [scsi_mod]
>> [   49.561613]  [] __scsi_remove_device+0xc3/0xd0 
>> [scsi_mod]
>> [   49.561619]  [] scsi_forget_host+0x52/0x63 [scsi_mod]
>> [   49.561623]  [] scsi_remove_host+0x8c/0x102 [scsi_mod]
>> [   49.561627]  [] usb_stor_disconnect+0x6b/0xab 
>> [usb_storage]
>> [   49.561634]  [] usb_unbind_interface+0x77/0x1ca 
>> [usbcore]
>> [   49.561636]  [] __device_release_driver+0x9d/0x121
>> [   49.561638]  [] device_release_driver+0x23/0x30
>> [   49.561639]  [] bus_remove_device+0xfb/0x10e
>> [   49.561641]  [] device_del+0x164/0x1e6
>> [   49.561648]  [] ? remove_intf_ep_devs+0x3b/0x48 
>> [usbcore]
>> [   49.561655]  [] usb_disable_device+0x84/0x1a5 [usbcore]
>> [   49.561661]  [] usb_disconnect+0x94/0x19f [usbcore]
>> [   49.561667]  [] hub_event+0x5c1/0xdea [usbcore]
>> [   49.561670]  [] process_one_work+0x1dc/0x37f
>> [   49.561672]  [] worker_thread+0x282/0x36d
>> [   49.561673]  [] ? rescuer_thread+0x2ae/0x2ae
>> [   49.561675]  [] kthread+0xd2/0xda
>> [   49.561678]  [] ret_from_fork+0x22/0x40
>> [   49.561679]  [] ? kthread_worker_fn+0x13e/0x13e
>>
>>   -ss
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 0734927..0c00928 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -1276,6 +1276,7 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
>  void __scsi_remove_device(struct scsi_device *sdev)
>  {
> struct device *dev = >sdev_gendev;
> +   struct scsi_target *starget;
>
> /*
>  * This cleanup path is not reentrant and while it is impossible
> @@ -1315,7 +1316,9 @@ void __scsi_remove_device(struct scsi_device *sdev)
>  * remoed sysfs visibility from the device, so make the target
>  * invisible if this was the last device underneath it.
>  */
> -   scsi_target_reap(scsi_target(sdev));
> +   starget = scsi_target(sdev);
> +   starget->state = STARGET_REMOVE;
> +   scsi_target_reap(starget);

Yes, scsi_target_reap is alse called from __scsi_remove_device,  and ->state is
_RUNNING here.


>
> put_device(dev);
>  }
>
>
>
> --
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/9] block: update chunk_sectors in blk_stack_limits()

[Bart Van Assche]

On 04/04/16 03:00, Hannes Reinecke wrote:

diff --git a/block/blk-settings.c b/block/blk-settings.c
index c7bb666..29fa900 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -630,6 +630,9 @@ int blk_stack_limits(struct queue_limits *t, struct 
queue_limits *b,
t->discard_granularity;
}

+   if (b->chunk_sectors)
+   t->chunk_sectors = max(t->chunk_sectors, b->chunk_sectors);
+
return ret;
  }
  EXPORT_SYMBOL(blk_stack_limits);


Hello Hannes,

My understanding is that a non-zero chunk_sectors value defines a 
maximum I/O size. Shouldn't max() be changed into min_not_zero()? From 
include/linux/blkdev.h:


static inline unsigned int blk_max_size_offset(struct request_queue *q,
   sector_t offset)
{
if (!q->limits.chunk_sectors)
return q->limits.max_sectors;

return q->limits.chunk_sectors -
(offset & (q->limits.chunk_sectors - 1));
}

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] scsi: Add intermediate STARGET_REMOVE state to scsi_target_state

[Xiong Zhou]

On Wed, Apr 13, 2016 at 4:19 PM, Johannes Thumshirn  wrote:
> Hi Xiong
> Sorry for the late reply
>
> On Dienstag, 12. April 2016 21:01:53 CEST Xiong Zhou wrote:
>> How about this?
>>
>> drivers/scsi/scsi_scan: mark STARGET_REMOVE state before destroy
>>
>> Signed-off-by: Xiong Zhou 
>> ---
>>  drivers/scsi/scsi_scan.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
>> index 27df7e7..21092e5 100644
>> --- a/drivers/scsi/scsi_scan.c
>> +++ b/drivers/scsi/scsi_scan.c
>> @@ -395,6 +395,8 @@ static void scsi_target_reap_ref_release(struct kref 
>> *kref)
>>   transport_remove_device(>dev);
>>   device_del(>dev);
>>   }
>> +
>> + starget->state = STARGET_REMOVE;
>>   scsi_target_destroy(starget);
>>  }
>
> The only scsi_target_reap_ref_release() caller is scsi_target_reap_ref_put(),
> which in turn is only called by scsi_target_reap(). scsi_target_reap()'s only
> callers are scsi_remove_target()/__scsi_remove_target(), which set the
> STARGET_REMOVE state and
>
> __scsi_add_device()
> scsi_get_host_dev()
> __scsi_scan_target()
>
> Which I'm currently investiganting (in parallel to reproducing the bug).

Thanks !

--
Xiong

>
>>
>> @@ -465,6 +467,7 @@ static struct scsi_target
>> *scsi_alloc_target(struct device *parent,
>>   dev_printk(KERN_ERR, dev, "target allocation failed, error %d\n", error);
>>   /* don't want scsi_target_reap to do the final
>>   * put because it will be under the host lock */
>> + starget->state = STARGET_REMOVE;
>>   scsi_target_destroy(starget);
>>   return NULL;
>>   }
>
> Here starget->state is STARGET_CREATED and the assertion is already aware of
> this state transitoin. IOTW this /shouldn't/ be needed.
>
>
> --
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mpt3sas: Remove usage of 'struct timeval'

[Martin K. Petersen]

> "Tina" == Tina Ruchandani  writes:

Tina> 'struct timeval' will have its tv_sec value overflow on 32-bit
Tina> systems in year 2038 and beyond. This patch replaces the use of
Tina> struct timeval for computing mpi_request.TimeStamp, and instead
Tina> uses ktime_t which provides 64-bit seconds value. The timestamp
Tina> computed remains unaffected (milliseconds since Unix epoch).

Broadcom folks, please review.

-- 
Martin K. Petersen  Oracle Linux Engineering
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mpt3sas: fix possible NULL dereference

[Martin K. Petersen]

> "Sudip" == Sudip Mukherjee  writes:

Sudip> We are dereferencing ioc->sense_dma_pool in pci_pool_free() and
Sudip> after that we are checking if it is NULL, before calling
Sudip> pci_pool_destroy().  Lets check if it is NULL before calling both
Sudip> pci_pool_free() and pci_pool_destroy().

Broadcom folks, please review.

-- 
Martin K. Petersen  Oracle Linux Engineering
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mpt3sas - remove unused fw_event_work delayed_work

[Martin K. Petersen]

> "Joe" == Joe Lawrence  writes:

Joe> Do we know why f1c35e6aea579 "mpt2sas: RESCAN Barrier work is added
Joe> in case of HBA reset" was unneeded for the mpt3 version?  If that
Joe> is interesting, that info could be added to v2 commit message as
Joe> well.

Chaitra?

-- 
Martin K. Petersen  Oracle Linux Engineering
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pm80xx: Remove bogus address masking in pm8001_ioremap()

[Martin K. Petersen]

> "David" == David Daney  writes:

David> It is unclear what the original intent of the masking was, but it
David> is clearly incorrect to truncate a physical address before
David> calling ioremap().  On systems where there are valid physical
David> address bits above bit-31 (arm64 for example) the result is an
David> eventual OOPs when initializing the driver.

David> Remove the bogus code to fix it.

Applied to 4.7/scsi-queue.

-- 
Martin K. Petersen  Oracle Linux Engineering
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH, RESEND 3] qla2xxx: Remove use of 'struct timeval'

[Martin K. Petersen]

> "Tina" == Tina Ruchandani  writes:

Tina,

>> Applied to 4.6/scsi-queue.

Tina> I am not seeing this patch in v4.6-rc3 in Linus's tree.

Not sure how I messed that up. Sorry!

Applied to 4.7/scsi-queue.

-- 
Martin K. Petersen  Oracle Linux Engineering
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] hisi_sas: v2 hw SATA fixes

[Martin K. Petersen]

> "John" == John Garry  writes:

John> This patchset introduces SATA support fixes for the HiSilicon v2
John> hw SAS controller.

John> Fixes include: - attach issue for SATA disk attached through
John> expander - intermittent issue for directly attaching multiple SATA
John> disks - add support for directly attaching SATA disk to phy index
John> 4+ - ITCT config issue

Applied to 4.7/scsi-queue.

-- 
Martin K. Petersen  Oracle Linux Engineering
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv3 1/2] target: make target db location configurable

[Lee Duncan]

This commit adds the read-write attribute "dbroot",
in the top-level CONFIGFS (core) target directory,
normally /sys/kernel/config/target. This attribute
defaults to "/var/target" but can be changed by
writing a new pathname string to it. Changing this
attribute is only allowed when no fabric drivers
are loaded and the supplied value specifies an
existing directory.

Target modules that care about the target database
root directory will be modified to use this
attribute in a future commit.

Signed-off-by: Lee Duncan 
---
 drivers/target/target_core_configfs.c | 62 +++
 drivers/target/target_core_internal.h |  6 
 2 files changed, 68 insertions(+)

diff --git a/drivers/target/target_core_configfs.c 
b/drivers/target/target_core_configfs.c
index 713c63d9681b..8cce79317971 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -99,6 +99,67 @@ static ssize_t target_core_item_version_show(struct 
config_item *item,
 
 CONFIGFS_ATTR_RO(target_core_item_, version);
 
+char db_root[DB_ROOT_LEN] = DB_ROOT_DEFAULT;
+static char db_root_stage[DB_ROOT_LEN];
+
+static ssize_t target_core_item_dbroot_show(struct config_item *item,
+   char *page)
+{
+   return sprintf(page, "%s\n", db_root);
+}
+
+static ssize_t target_core_item_dbroot_store(struct config_item *item,
+   const char *page, size_t count)
+{
+   ssize_t read_bytes;
+   struct file *fp;
+
+   mutex_lock(_tf_lock);
+   if (!list_empty(_tf_list)) {
+   mutex_unlock(_tf_lock);
+   pr_err("db_root: cannot be changed: target drivers registered");
+   return -EINVAL;
+   }
+
+   if (count > (DB_ROOT_LEN - 1)) {
+   mutex_unlock(_tf_lock);
+   pr_err("db_root: count %d exceeds DB_ROOT_LEN-1: %u\n",
+  (int)count, DB_ROOT_LEN - 1);
+   return -EINVAL;
+   }
+
+   read_bytes = snprintf(db_root_stage, DB_ROOT_LEN, "%s", page);
+   if (!read_bytes) {
+   mutex_unlock(_tf_lock);
+   return -EINVAL;
+   }
+   if (db_root_stage[read_bytes - 1] == '\n')
+   db_root_stage[read_bytes - 1] = '\0';
+
+   /* validate new db root before accepting it */
+   fp = filp_open(db_root_stage, O_RDONLY, 0);
+   if (IS_ERR(fp)) {
+   mutex_unlock(_tf_lock);
+   pr_err("db_root: cannot open: %s\n", db_root_stage);
+   return -EINVAL;
+   }
+   if (!S_ISDIR(fp->f_inode->i_mode)) {
+   filp_close(fp, 0);
+   mutex_unlock(_tf_lock);
+   pr_err("db_root: not a directory: %s\n", db_root_stage);
+   return -EINVAL;
+   }
+   filp_close(fp, 0);
+
+   strncpy(db_root, db_root_stage, read_bytes);
+
+   mutex_unlock(_tf_lock);
+
+   return read_bytes;
+}
+
+CONFIGFS_ATTR(target_core_item_, dbroot);
+
 static struct target_fabric_configfs *target_core_get_fabric(
const char *name)
 {
@@ -249,6 +310,7 @@ static struct configfs_group_operations 
target_core_fabric_group_ops = {
  */
 static struct configfs_attribute *target_core_fabric_item_attrs[] = {
_core_item_attr_version,
+   _core_item_attr_dbroot,
NULL,
 };
 
diff --git a/drivers/target/target_core_internal.h 
b/drivers/target/target_core_internal.h
index 040cf5202e54..c2a18b960c5d 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -156,4 +156,10 @@ void   
target_stat_setup_mappedlun_default_groups(struct se_lun_acl *);
 /* target_core_xcopy.c */
 extern struct se_portal_group xcopy_pt_tpg;
 
+/* target_core_configfs.c */
+#define DB_ROOT_LEN4096
+#defineDB_ROOT_DEFAULT "/var/target"
+
+extern char db_root[];
+
 #endif /* TARGET_CORE_INTERNAL_H */
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv3 2/2] target: use new "dbroot" target attribute

[Lee Duncan]

This commit updates the target core ALUA and PR
modules to use the new "dbroot" attribute instead
of assuming the target database is in "/var/target".

Signed-off-by: Lee Duncan 
Reviewed-by: Hannes Reinecke 
---
 drivers/target/target_core_alua.c | 6 +++---
 drivers/target/target_core_pr.c   | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_alua.c 
b/drivers/target/target_core_alua.c
index 49aba4a31747..4c82bbe19003 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -932,7 +932,7 @@ static int core_alua_update_tpg_primary_metadata(
tg_pt_gp->tg_pt_gp_alua_access_status);
 
snprintf(path, ALUA_METADATA_PATH_LEN,
-   "/var/target/alua/tpgs_%s/%s", >unit_serial[0],
+   "%s/alua/tpgs_%s/%s", db_root, >unit_serial[0],
config_item_name(_pt_gp->tg_pt_gp_group.cg_item));
 
rc = core_alua_write_tpg_metadata(path, md_buf, len);
@@ -1275,8 +1275,8 @@ static int core_alua_update_tpg_secondary_metadata(struct 
se_lun *lun)
atomic_read(>lun_tg_pt_secondary_offline),
lun->lun_tg_pt_secondary_stat);
 
-   snprintf(path, ALUA_METADATA_PATH_LEN, 
"/var/target/alua/%s/%s/lun_%llu",
-   se_tpg->se_tpg_tfo->get_fabric_name(), wwn,
+   snprintf(path, ALUA_METADATA_PATH_LEN, "%s/alua/%s/%s/lun_%llu",
+   db_root, se_tpg->se_tpg_tfo->get_fabric_name(), wwn,
lun->unpacked_lun);
 
rc = core_alua_write_tpg_metadata(path, md_buf, len);
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index b1795735eafc..47463c99c318 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -1985,7 +1985,7 @@ static int __core_scsi3_write_aptpl_to_file(
return -EMSGSIZE;
}
 
-   snprintf(path, 512, "/var/target/pr/aptpl_%s", >unit_serial[0]);
+   snprintf(path, 512, "%s/pr/aptpl_%s", db_root, >unit_serial[0]);
file = filp_open(path, flags, 0600);
if (IS_ERR(file)) {
pr_err("filp_open(%s) for APTPL metadata"
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv3 0/2] target: make location of /var/targets configurable

[Lee Duncan]

These patches make the location of "/var/target" configurable,
though it still defauls to "/var/target".

This "target database directory" can only be changed
after the target_core_mod loads but before any
fabric drivers are loaded, and must be the pathname
of an existing directory.

This configuration is accomplished via the configfs
top-level target attribute "dbroot", i.e. dumping
out "/sys/kernel/config/target/dbroot" will normally
return "/var/target". Writing to this attribute
changes the loation where the kernel looks for the
target database.

The first patch creates this configurable value for
the "dbroot", and the second patch modifies users
of this directory to use this new attribute.

Changes from v2:
 * Add locking around access to target driver list

Changes from v1:
 * Only allow changing target DB root before it
   can be used by others
 * Validate that new DB root is a valid directory

Lee Duncan (2):
  target: make target db location configurable
  target: use new "dbroot" target attribute

 drivers/target/target_core_alua.c |  6 ++--
 drivers/target/target_core_configfs.c | 62 +++
 drivers/target/target_core_internal.h |  6 
 drivers/target/target_core_pr.c   |  2 +-
 4 files changed, 72 insertions(+), 4 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 1/2] target: make target db location configurable

[Lee Duncan]

On 04/13/2016 11:10 PM, Hannes Reinecke wrote:
> On 04/13/2016 10:25 PM, Lee Duncan wrote:
>> This commit adds the read-write attribute "dbroot",
>> in the top-level CONFIGFS (core) target directory,
>> normally /sys/kernel/config/target. This attribute
>> defaults to "/var/target" but can be changed by
>> writing a new pathname string to it. Changing this
>> attribute is only allowed when no fabric drivers
>> are loaded and the supplied value specifies an
>> existing directory.
>>
>> Target modules that care about the target database
>> root directory will be modified to use this
>> attribute in a future commit.
>>
>> Signed-off-by: Lee Duncan 
>> ---
>>  drivers/target/target_core_configfs.c | 51 
>> +++
>>  drivers/target/target_core_internal.h |  6 +
>>  2 files changed, 57 insertions(+)
>>
>> diff --git a/drivers/target/target_core_configfs.c 
>> b/drivers/target/target_core_configfs.c
>> index 713c63d9681b..bfedbd92b77f 100644
>> --- a/drivers/target/target_core_configfs.c
>> +++ b/drivers/target/target_core_configfs.c
>> @@ -99,6 +99,56 @@ static ssize_t target_core_item_version_show(struct 
>> config_item *item,
>>  
>>  CONFIGFS_ATTR_RO(target_core_item_, version);
>>  
>> +char db_root[DB_ROOT_LEN] = DB_ROOT_DEFAULT;
>> +static char db_root_stage[DB_ROOT_LEN];
>> +
>> +static ssize_t target_core_item_dbroot_show(struct config_item *item,
>> +char *page)
>> +{
>> +return sprintf(page, "%s\n", db_root);
>> +}
>> +
>> +static ssize_t target_core_item_dbroot_store(struct config_item *item,
>> +const char *page, size_t count)
>> +{
>> +ssize_t read_bytes;
>> +struct file *fp;
>> +
>> +if (!list_empty(_tf_list)) {
>> +pr_err("db_root: cannot be changed: target drivers registered");
>> +return -EINVAL;
>> +}
> Locking?

Doh. I will resubmit with locking shortly.

> 
> Cheers,
> 
> Hannes
> 

-- 
Lee Duncan

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/9] blk-sysfs: Add 'chunk_sectors' to sysfs attributes

[Bart Van Assche]

On 04/04/2016 03:00 AM, Hannes Reinecke wrote:

The queue limits already have a 'chunk_sectors' setting, so
we should be presenting it via sysfs.


This patch does more than exporting chunk_sectors via sysfs. It also 
makes that parameter configurable. Please mention this in the patch 
description.


My understanding of the block drivers that call 
blk_queue_chunk_sectors() is that increasing the chunk_sectors parameter 
will break these drivers. I think the single queue_limits.chunk_sectors 
parameter needs to be converted into two parameters:

- The value set by the block driver by calling
  blk_queue_chunk_sectors().
- The value configured from user space through sysfs.

This will allow to ensure that the chunk_sectors parameter can be 
increased from user space and also that it cannot be decreased.



+static ssize_t
+queue_chunk_sectors_store(struct request_queue *q, const char *page, size_t 
count)
+{
+   unsigned long chunk_sectors;
+
+   ssize_t ret = queue_var_store(_sectors, page, count);
+   if (ret < 0)
+   return ret;
+   spin_lock_irq(q->queue_lock);
+   blk_queue_chunk_sectors(q, chunk_sectors);
+   spin_unlock_irq(q->queue_lock);
+
+   return ret;
+}


In blk_queue_chunk_sectors() I found the following:

BUG_ON(!is_power_of_2(chunk_sectors));

Please make sure that this BUG_ON() cannot be triggered from user space.

Additionally, an update for Documentation/block/queue-sysfs.txt is 
missing from this patch.


Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch v2] scsi_dh_alua: uninitialized variable in alua_rtpg()

[Bart Van Assche]

On 04/14/2016 11:20 AM, Dan Carpenter wrote:

It's possible to use "err" without initializing it.  If it happens to be
a 2 which is SCSI_DH_RETRY then that could cause a bug.  Bart Van Assche
pointed out that we should probably re-initialize it for every iteration
through the retry loop.

Signed-off-by: Dan Carpenter 
---
v2: The first version just initialized it at the start of the function.

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c 
b/drivers/scsi/device_handler/scsi_dh_alua.c
index 8eaed05..a655cf2 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -532,6 +532,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct 
alua_port_group *pg)
return SCSI_DH_DEV_TEMP_BUSY;

   retry:
+   err = 0;
retval = submit_rtpg(sdev, buff, bufflen, _hdr, pg->flags);

if (retval) {


Although I would have preferred that that initialization would have been 
closer to the other 'err' assignments this patch looks fine to me. If 
this patch does not get integrated in kernel v4.6 a "Cc: stable" tag 
will be needed.


Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] scsi_dh_alua: uninitialized variable in alua_rtpg()

[Dan Carpenter]

On Thu, Apr 14, 2016 at 08:45:18AM -0700, Bart Van Assche wrote:
> On 04/14/2016 02:39 AM, Dan Carpenter wrote:
> >It's possible to use "err" without initializing it.  If it happens to be
> >a 2 which is SCSI_DH_RETRY then that could cause a bug.
> >
> >Signed-off-by: Dan Carpenter 
> >
> >diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c 
> >b/drivers/scsi/device_handler/scsi_dh_alua.c
> >index 8eaed05..f3c994f 100644
> >--- a/drivers/scsi/device_handler/scsi_dh_alua.c
> >+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> >@@ -513,7 +513,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct 
> >alua_port_group *pg)
> > struct alua_port_group *tmp_pg;
> > int len, k, off, valid_states = 0, bufflen = ALUA_RTPG_SIZE;
> > unsigned char *desc, *buff;
> >-unsigned err, retval;
> >+unsigned int err = 0;
> >+unsigned int retval;
> > unsigned int tpg_desc_tbl_off;
> > unsigned char orig_transition_tmo;
> > unsigned long flags;
> 
> Hello Dan,
> 
> The code that uses the 'err' variable occurs in a loop. I think the
> initialization of 'err' should occur after the "retry:" label.

It looks like you're right.  I'll resend.  I don't know this code very
well, obviously and it's a static checker fix not something I have
tested.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[patch v2] scsi_dh_alua: uninitialized variable in alua_rtpg()

[Dan Carpenter]

It's possible to use "err" without initializing it.  If it happens to be
a 2 which is SCSI_DH_RETRY then that could cause a bug.  Bart Van Assche
pointed out that we should probably re-initialize it for every iteration
through the retry loop.

Signed-off-by: Dan Carpenter 
---
v2: The first version just initialized it at the start of the function.

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c 
b/drivers/scsi/device_handler/scsi_dh_alua.c
index 8eaed05..a655cf2 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -532,6 +532,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct 
alua_port_group *pg)
return SCSI_DH_DEV_TEMP_BUSY;
 
  retry:
+   err = 0;
retval = submit_rtpg(sdev, buff, bufflen, _hdr, pg->flags);
 
if (retval) {
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] scsi_dh_alua: Declare local functions static

[Bart Van Assche]

This patch avoids that building with W=1 causes gcc to report the
following type of warning:

no previous prototype for ... [-Wmissing-prototypes]

Signed-off-by: Bart Van Assche 
Cc: Hannes Reinecke 
Cc: Christoph Hellwig 
Cc: Ewan Milne 
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c 
b/drivers/scsi/device_handler/scsi_dh_alua.c
index 8eaed05..e034f12 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -190,8 +190,8 @@ static int submit_stpg(struct scsi_device *sdev, int 
group_id,
  ALUA_FAILOVER_RETRIES, NULL, req_flags);
 }
 
-struct alua_port_group *alua_find_get_pg(char *id_str, size_t id_size,
-int group_id)
+static struct alua_port_group *alua_find_get_pg(char *id_str, size_t id_size,
+   int group_id)
 {
struct alua_port_group *pg;
 
@@ -219,8 +219,8 @@ struct alua_port_group *alua_find_get_pg(char *id_str, 
size_t id_size,
  * Allocate a new port_group structure for a given
  * device.
  */
-struct alua_port_group *alua_alloc_pg(struct scsi_device *sdev,
- int group_id, int tpgs)
+static struct alua_port_group *alua_alloc_pg(struct scsi_device *sdev,
+int group_id, int tpgs)
 {
struct alua_port_group *pg, *tmp_pg;
 
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 05/14] libata: NCQ Encapsulation for READ LOG DMA EXT

[Tejun Heo]

On Thu, Apr 14, 2016 at 05:59:48PM +0200, Hannes Reinecke wrote:
> For this patch, yes, you are right.
> However, the ZAC enablement patches later on submit READ LOG EXT commands
> (for REPORT ZONES), and _they_ benefit from NCQ encapsulation.

Umm... so, you can't use ata_exec_internal() outside of EH context.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 05/14] libata: NCQ Encapsulation for READ LOG DMA EXT

[Hannes Reinecke]

On 04/14/2016 05:43 PM, Tejun Heo wrote:

Hello, Hannes.

On Thu, Apr 14, 2016 at 07:44:19AM +0200, Hannes Reinecke wrote:

Hehe. No, it isn't, if you look closely.
(Or make that: it _shouldn't_, and I've messed it up)
(That's what the 'fpdma' parameter is for)

The benefit is not so much for normal operations, but it'll give us
a performance improvements on SMR drives where we need to issue
READ LOG DMA EXT rather frequently. There we really want to have
them as NCQ commands.


I'm still a bit confused.  Isn't it part of EH?  If so, the path is
never traveled with other commands in flight and thus whether a
command is NCQ or not doesn't make any difference.  The only thing
it'd do is making devices which advertise the capability but don't get
it quite right fail.


For this patch, yes, you are right.
However, the ZAC enablement patches later on submit READ LOG EXT 
commands (for REPORT ZONES), and _they_ benefit from NCQ encapsulation.


Cheers,

Hannes

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)

[Josh Poimboeuf]

On Thu, Apr 14, 2016 at 05:29:06PM +0200, Denys Vlasenko wrote:
> On 04/13/2016 07:10 PM, Josh Poimboeuf wrote:
> >> From the disassembly of drivers/scsi/qla2xxx/qla_attr.o:
> >>
> >> 2f53 :
> >> 2f53:   55  push   %rbp
> >> 2f54:   48 89 e5mov%rsp,%rbp
> >>
> >> 2f57 :
> >> 2f57:   55  push   %rbp
> >> 2f58:   b9 e8 00 00 00  mov$0xe8,%ecx
> >> 2f5d:   48 89 e5mov%rsp,%rbp
> >> ...
> >>
> >> Note that qla2x00_get_host_fabric_name() is inexplicably
> >> truncated after
> >> setting up the frame pointer.  It falls through to the next
> >> function, which is
> >> very wrong.
> >
> > Wow, that's ... interesting.
> >
> >
> >> I can recreate it with either gcc 5.3.1 or gcc 6.0 on
> >> linus/master with
> >> the .config from the above link.
> >>
> >> The call chain which appears to trigger the problem is:
> >>
> >> qla2x00_get_host_fabric_name()
> >>   wwn_to_u64()
> >> get_unaligned_be64()
> >>   be64_to_cpup()
> >> __be64_to_cpup() <- changed to __always_inline by this
> >> patch
> >>
> >> It occurs with the combination of the following two recent
> >> commits:
> >>
> >> - bc27fb68aaad ("include/uapi/linux/byteorder, swab: force
> >> inlining of some byteswap operations")
> >> - ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn
> >> access")
> >>
> >> I can confirm that reverting either patch makes the problem go
> >> away.
> >> I'm planning on opening a gcc bug tomorrow.
> >
> >
> > Note that if CONFIG_OPTIMIZE_INLINING is not set, _all_ "inline"
> > keywords are in fact __always_inline, so the bug must be
> > triggering
> > even without the patch.
> 
>  Makes sense in theory, but the bug doesn't actually trigger when I
>  revert the patch and set CONFIG_OPTIMIZE_INLINING=n.
> 
>  Perhaps even more surprising, it doesn't trigger *with* the patch
>  and
>  CONFIG_OPTIMIZE_INLINING=n.
> >>>
> >>> [ Adding James to CC since this bug affects scsi. ]
> >>>
> >>> Here's the gcc bug:
> >>>
> >>>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
> >>>
> >>
> >>
> >> Actually, adding me doesn't help, I've added linux-scsi.  The summary
> >> is that there's a but in gcc-5.3.1 which is miscompiling qla_attr.c ...
> >> this means we're going to have to ask the compiler version of reported
> >> crashes.
> > 
> > The bug isn't specific to a compiler version.  I've seen it with gcc
> > 5.3.1 and gcc 6.0.  I haven't tried any older versions.  And the gcc bug
> > hasn't been resolved (or even investigated) yet.
> > 
> > The bug is triggered by a combination of the above two commits from the
> > 4.6 merge window, so presumably we'd need to revert one of them to avoid
> > crashes in 4.6.
> 
> The bug is indeed in the compiler. 4.9 and all later versions are affected.
> gcc bugzilla now has a reproducer. In abridged form:
> 
> 
> static inline __attribute__((always_inline)) u64 __swab64p(const u64 *p)
> {
>  return (__builtin_constant_p((u64)(*p)) ? ((u64)( (((u64)(*p) & 
> (u64)0x00ffULL) << 56) | (((u64)(*p) & 
> (u64)0xff00ULL) << 40) | (((u64)(*p) & 
> (u64)0x00ffULL) << 24) | (((u64)(*p) & 
> (u64)0xff00ULL) << 8) | (((u64)(*p) & (u64)0x00ffULL) 
> >> 8) | (((u64)(*p) & (u64)0xff00ULL) >> 24) | (((u64)(*p) & 
> (u64)0x00ffULL) >> 40) | (((u64)(*p) & 
> (u64)0xff00ULL) >> 56))) : __builtin_bswap64(*p));
> }
> static inline u64 wwn_to_u64(void *wwn)
> {
>  return __swab64p(wwn);
> }
> static void qla2x00_get_host_fabric_name(struct Scsi_Host *shost)
> {
>  scsi_qla_host_t *vha = shost_priv(shost);
>  u8 node_name[8] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF};
>  u64 fabric_name = wwn_to_u64(node_name);
>  if (vha->device_flags & 0x1)
>   fabric_name = wwn_to_u64(vha->fabric_node_name);
>  (((struct fc_host_attrs *)(shost)->shost_data)->fabric_name) = fabric_name;
> }

Nice work with the reproducer!

> Two (or more, there were more before simplification) levels of inlining
> are necessary for bug to trigger in this example (folding to one level
> makes it go away). "__attribute__((always_inline))" is necessary too.
> 
> 
> Since we have lots of __always_inline anyway, this bug has a potential
> to miscompile kernels regardless of CONFIG_OPTIMIZE_INLINING setting,
> and with or without the patches mentioned above (they just happen
> to create a reliable reproducer).

Well, but setting !CONFIG_OPTIMIZE_INLINING makes the problem go away
for some reason.  It seems like the bug requires wwn_to_u64() being
out-of-line and __swab64p() being in-line.

In fact, the following patch seems to fix it:

diff --git 

Re: [patch] scsi_dh_alua: uninitialized variable in alua_rtpg()

[Bart Van Assche]

On 04/14/2016 02:39 AM, Dan Carpenter wrote:

It's possible to use "err" without initializing it.  If it happens to be
a 2 which is SCSI_DH_RETRY then that could cause a bug.

Signed-off-by: Dan Carpenter 

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c 
b/drivers/scsi/device_handler/scsi_dh_alua.c
index 8eaed05..f3c994f 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -513,7 +513,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct 
alua_port_group *pg)
struct alua_port_group *tmp_pg;
int len, k, off, valid_states = 0, bufflen = ALUA_RTPG_SIZE;
unsigned char *desc, *buff;
-   unsigned err, retval;
+   unsigned int err = 0;
+   unsigned int retval;
unsigned int tpg_desc_tbl_off;
unsigned char orig_transition_tmo;
unsigned long flags;


Hello Dan,

The code that uses the 'err' variable occurs in a loop. I think the 
initialization of 'err' should occur after the "retry:" label.


Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 09/14] libata: fixup ZAC device disabling

[Tejun Heo]

On Thu, Apr 14, 2016 at 07:48:03AM +0200, Hannes Reinecke wrote:
> On 04/13/2016 08:09 PM, Tejun Heo wrote:
> > On Tue, Apr 12, 2016 at 08:47:53AM +0200, Hannes Reinecke wrote:
> >> libata device disabling is ... curious. So add the correct
> >> definitions that we can disable ZAC devices properly.
> > 
> > Fold into an earlier patch?
> > 
> That earlier patch would be commit
> 9162c6579bf90b3f5ddb7e3a6c6fa946c1b4cbeb
> ("libata: Implement ATA_DEV_ZAC"), which got merged
> in 2014 ...

Ah, right you are.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 05/14] libata: NCQ Encapsulation for READ LOG DMA EXT

[Tejun Heo]

Hello, Hannes.

On Thu, Apr 14, 2016 at 07:44:19AM +0200, Hannes Reinecke wrote:
> Hehe. No, it isn't, if you look closely.
> (Or make that: it _shouldn't_, and I've messed it up)
> (That's what the 'fpdma' parameter is for)
> 
> The benefit is not so much for normal operations, but it'll give us
> a performance improvements on SMR drives where we need to issue
> READ LOG DMA EXT rather frequently. There we really want to have
> them as NCQ commands.

I'm still a bit confused.  Isn't it part of EH?  If so, the path is
never traveled with other commands in flight and thus whether a
command is NCQ or not doesn't make any difference.  The only thing
it'd do is making devices which advertise the capability but don't get
it quite right fail.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)

[Denys Vlasenko]

On 04/13/2016 07:10 PM, Josh Poimboeuf wrote:
>> From the disassembly of drivers/scsi/qla2xxx/qla_attr.o:
>>
>> 2f53 :
>> 2f53:   55  push   %rbp
>> 2f54:   48 89 e5mov%rsp,%rbp
>>
>> 2f57 :
>> 2f57:   55  push   %rbp
>> 2f58:   b9 e8 00 00 00  mov$0xe8,%ecx
>> 2f5d:   48 89 e5mov%rsp,%rbp
>> ...
>>
>> Note that qla2x00_get_host_fabric_name() is inexplicably
>> truncated after
>> setting up the frame pointer.  It falls through to the next
>> function, which is
>> very wrong.
>
> Wow, that's ... interesting.
>
>
>> I can recreate it with either gcc 5.3.1 or gcc 6.0 on
>> linus/master with
>> the .config from the above link.
>>
>> The call chain which appears to trigger the problem is:
>>
>> qla2x00_get_host_fabric_name()
>>   wwn_to_u64()
>> get_unaligned_be64()
>>   be64_to_cpup()
>> __be64_to_cpup() <- changed to __always_inline by this
>> patch
>>
>> It occurs with the combination of the following two recent
>> commits:
>>
>> - bc27fb68aaad ("include/uapi/linux/byteorder, swab: force
>> inlining of some byteswap operations")
>> - ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn
>> access")
>>
>> I can confirm that reverting either patch makes the problem go
>> away.
>> I'm planning on opening a gcc bug tomorrow.
>
>
> Note that if CONFIG_OPTIMIZE_INLINING is not set, _all_ "inline"
> keywords are in fact __always_inline, so the bug must be
> triggering
> even without the patch.

 Makes sense in theory, but the bug doesn't actually trigger when I
 revert the patch and set CONFIG_OPTIMIZE_INLINING=n.

 Perhaps even more surprising, it doesn't trigger *with* the patch
 and
 CONFIG_OPTIMIZE_INLINING=n.
>>>
>>> [ Adding James to CC since this bug affects scsi. ]
>>>
>>> Here's the gcc bug:
>>>
>>>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
>>>
>>
>>
>> Actually, adding me doesn't help, I've added linux-scsi.  The summary
>> is that there's a but in gcc-5.3.1 which is miscompiling qla_attr.c ...
>> this means we're going to have to ask the compiler version of reported
>> crashes.
> 
> The bug isn't specific to a compiler version.  I've seen it with gcc
> 5.3.1 and gcc 6.0.  I haven't tried any older versions.  And the gcc bug
> hasn't been resolved (or even investigated) yet.
> 
> The bug is triggered by a combination of the above two commits from the
> 4.6 merge window, so presumably we'd need to revert one of them to avoid
> crashes in 4.6.

The bug is indeed in the compiler. 4.9 and all later versions are affected.
gcc bugzilla now has a reproducer. In abridged form:


static inline __attribute__((always_inline)) u64 __swab64p(const u64 *p)
{
 return (__builtin_constant_p((u64)(*p)) ? ((u64)( (((u64)(*p) & 
(u64)0x00ffULL) << 56) | (((u64)(*p) & (u64)0xff00ULL) 
<< 40) | (((u64)(*p) & (u64)0x00ffULL) << 24) | (((u64)(*p) & 
(u64)0xff00ULL) << 8) | (((u64)(*p) & (u64)0x00ffULL) 
>> 8) | (((u64)(*p) & (u64)0xff00ULL) >> 24) | (((u64)(*p) & 
(u64)0x00ffULL) >> 40) | (((u64)(*p) & (u64)0xff00ULL) 
>> 56))) : __builtin_bswap64(*p));
}
static inline u64 wwn_to_u64(void *wwn)
{
 return __swab64p(wwn);
}
static void qla2x00_get_host_fabric_name(struct Scsi_Host *shost)
{
 scsi_qla_host_t *vha = shost_priv(shost);
 u8 node_name[8] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF};
 u64 fabric_name = wwn_to_u64(node_name);
 if (vha->device_flags & 0x1)
  fabric_name = wwn_to_u64(vha->fabric_node_name);
 (((struct fc_host_attrs *)(shost)->shost_data)->fabric_name) = fabric_name;
}


Two (or more, there were more before simplification) levels of inlining
are necessary for bug to trigger in this example (folding to one level
makes it go away). "__attribute__((always_inline))" is necessary too.


Since we have lots of __always_inline anyway, this bug has a potential
to miscompile kernels regardless of CONFIG_OPTIMIZE_INLINING setting,
and with or without the patches mentioned above (they just happen
to create a reliable reproducer).

Since it was not detected for two years since gcc 4.9 release,
it must be triggering quite rarely.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [SCSI] pm80xx: Phy settings support for motherboard controller.

[Jack Wang]

2016-04-13 13:24 GMT+02:00 Dan Carpenter :
> Hello Anand Kumar Santhanam,
>
> The patch 279094079a44: "[SCSI] pm80xx: Phy settings support for
> motherboard controller." from Sep 18, 2013, leads to the following
> static checker warning:
>
> drivers/scsi/pm8001/pm80xx_hwi.c:4554 mpi_set_phy_profile_req()
> error: uninitialized symbol 'tag'.
>
Thanks for reporting, attached patch should fix the warning.
From e30af801d9ee9979b2a7a2af815cb395c2255a09 Mon Sep 17 00:00:00 2001
From: Jack Wang 
Date: Thu, 14 Apr 2016 14:38:57 +0200
Subject: [PATCH] pm80xx: avoid to use invalid tag

Fix static checker warning:

drivers/scsi/pm8001/pm80xx_hwi.c:4554 mpi_set_phy_profile_req()
error: uninitialized symbol 'tag'.

Reported-by: Dan Carpenter dan.carpen...@oracle.com
Signed-off-by: Jack Wang 
---
 drivers/scsi/pm8001/pm80xx_hwi.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
index eb4fee6..a3c1c08 100644
--- a/drivers/scsi/pm8001/pm80xx_hwi.c
+++ b/drivers/scsi/pm8001/pm80xx_hwi.c
@@ -4548,8 +4548,10 @@ void mpi_set_phy_profile_req(struct pm8001_hba_info *pm8001_ha,
 
 	memset(, 0, sizeof(payload));
 	rc = pm8001_tag_alloc(pm8001_ha, );
-	if (rc)
+	if (rc) {
 		PM8001_FAIL_DBG(pm8001_ha, pm8001_printk("Invalid tag\n"));
+		return;
+	}
 	circularQ = _ha->inbnd_q_tbl[0];
 	payload.tag = cpu_to_le32(tag);
 	payload.ppc_phyid = (((operation & 0xF) << 8) | (phyid  & 0xFF));
@@ -4590,8 +4592,10 @@ void pm8001_set_phy_profile_single(struct pm8001_hba_info *pm8001_ha,
 	memset(, 0, sizeof(payload));
 
 	rc = pm8001_tag_alloc(pm8001_ha, );
-	if (rc)
+	if (rc) {
 		PM8001_INIT_DBG(pm8001_ha, pm8001_printk("Invalid tag"));
+		return;
+	}
 
 	circularQ = _ha->inbnd_q_tbl[0];
 	opc = OPC_INB_SET_PHY_PROFILE;
-- 
1.9.1

Re: [PATCH] pm80xx: Remove bogus address masking in pm8001_ioremap()

[Jinpu Wang]

On Wed, Apr 13, 2016 at 11:26 PM, David Daney  wrote:
> From: David Daney 
>
> It is unclear what the original intent of the masking was, but it is
> clearly incorrect to truncate a physical address before calling
> ioremap().  On systems where there are valid physical address bits
> above bit-31 (arm64 for example) the result is an eventual OOPs when
> initializing the driver.
>
> Remove the bogus code to fix it.
>
> Signed-off-by: David Daney 
> ---
>  drivers/scsi/pm8001/pm8001_init.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_init.c 
> b/drivers/scsi/pm8001/pm8001_init.c
> index 062ab34..6bd7bf4 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -418,8 +418,6 @@ static int pm8001_ioremap(struct pm8001_hba_info 
> *pm8001_ha)
> if (pci_resource_flags(pdev, bar) & IORESOURCE_MEM) {
> pm8001_ha->io_mem[logicalBar].membase =
> pci_resource_start(pdev, bar);
> -   pm8001_ha->io_mem[logicalBar].membase &=
> -   (u32)PCI_BASE_ADDRESS_MEM_MASK;
> pm8001_ha->io_mem[logicalBar].memsize =
> pci_resource_len(pdev, bar);
> pm8001_ha->io_mem[logicalBar].memvirtaddr =
> --
> 1.8.3.1
>

Thanks, looks good to me.
Acked-by: Jack Wang 
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 09/27] target: avoid to access .bi_vcnt directly

[Ming Lei]

When the bio is full, bio_add_pc_page() will return zero,
so use this way to handle full bio.

Also replace access to .bi_vcnt for pr_debug() with bio_segments().

Signed-off-by: Ming Lei 
---
 drivers/target/target_core_pscsi.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/target/target_core_pscsi.c 
b/drivers/target/target_core_pscsi.c
index de18790..66320e8 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -945,13 +945,9 @@ pscsi_map_sg(struct se_cmd *cmd, struct scatterlist *sgl, 
u32 sgl_nents,
 
rc = bio_add_pc_page(pdv->pdv_sd->request_queue,
bio, page, bytes, off);
-   if (rc != bytes)
-   goto fail;
-
pr_debug("PSCSI: bio->bi_vcnt: %d nr_vecs: %d\n",
-   bio->bi_vcnt, nr_vecs);
-
-   if (bio->bi_vcnt > nr_vecs) {
+   bio_segments(bio), nr_vecs);
+   if (rc != bytes) {
pr_debug("PSCSI: Reached bio->bi_vcnt max:"
" %d i: %d bio: %p, allocating another"
" bio\n", bio->bi_vcnt, i, bio);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 00/27] block: cleanup direct access to .bi_vcnt & .bi_io_vec

[Ming Lei]

Hi Guys,

It is always not a good practice to access bio->bi_vcnt and
bio->bi_io_vec from drivers directly. Also this kind of direct
access will cause trouble when converting to multipage bvecs
because currently drivers may suppose one bvec always include
one page, and use the two fields to figure out how to handle
pages.

Even the actual meaning of the two fields arn't change from
block subsystem's view, but it may change from driver or fs's
view, so this patchset takes a conservative approach to cleanup
direct access to the two fields for avoiding regressions.

The 1st patch introduces the following 3 bio helpers which can be
used inside drivers for avoiding direct access to .bi_vcnt and .bi_io_vec.

bio_pages()
bio_get_base_vec()
bio_set_vec_table()

bio_pages() can be easy to convert to multipage bvecs.

For bio_get_base_vec() and bio_set_vec_table(), they are often used
during initializing a new bio or in case of single bvec bio. With the
two new helpers, it becomes quite easy to audit access to .bi_io_vec
and .bi_vcnt.

Most of the other patches use the 3 helpers to clean up most of direct
access to .bi_vcnt and .bi_io_vec from drivers, except for MD and btrfs,
which two subsystems will be handled with different way in future.

For btrfs, its direct access to .bi_vcnt & .bi_io_vec need to be cleanuped
and audited that there isn't issue once converting to multipage bvecs.

For raid(md), given its usage is quite complicated, we can just
not enable multipage bvecs for raid queue until all its usage
are cleaned up and audited. So it won't be a blocker for multipage
bvecs.

Also bio_add_page() is used in floppy, dm-crypt and fs/logfs to
avoiding direct access to .bi_vcnt & .bi_io_vec.

The patchset can be found in the following tree:

https://github.com/ming1/linux/tree/v4.6-rc-block-next-mpbvecs-cleanup.v1

V1:
- add Reviewed-by
- remove bio_is_full() helper because target can find it
via the return value of bio_add_pc_page() (9/27)
- add comment on another two uses of bio_get_base_vec() (16/27)
- rebased on latest for-next branch of block tree

Ming Lei (27):
  block: bio: introduce 3 helpers for cleanup
  block: drbd: use bio_get_base_vec() to retrieve the 1st bvec
  block: drbd: remove impossible failure handling
  block: loop: use bio_get_base_vec() to retrive bvec table
  block: pktcdvd: use bio_get_base_vec() to retrive bvec table
  block: floppy: use bio_set_vec_table()
  block: floppy: use bio_add_page()
  staging: lustre: avoid to use bio->bi_vcnt directly
  target: avoid to access .bi_vcnt directly
  bcache: debug: avoid to access .bi_io_vec directly
  bcache: io.c: use bio_set_vec_table
  bcache: journal.c: use bio_set_vec_table()
  bcache: movinggc: use bio_set_vec_table()
  bcache: writeback: use bio_set_vec_table()
  bcache: super: use bio_set_vec_table()
  bcache: super: use bio_get_base_vec
  dm: crypt: use bio_add_page()
  dm: dm-io.c: use bio_get_base_vec()
  dm: dm.c: replace 'bio->bi_vcnt == 1' with !bio_multiple_segments
  dm: dm-bufio.c: use bio_set_vec_table()
  fs: logfs: use bio_set_vec_table()
  fs: logfs: convert to bio_add_page() in sync_request()
  fs: logfs: use bio_add_page() in __bdev_writeseg()
  fs: logfs: use bio_add_page() in do_erase()
  fs: logfs: remove unnecesary check
  kernel/power/swap.c: use bio_get_base_vec()
  mm: page_io.c: use bio_get_base_vec()

 drivers/block/drbd/drbd_bitmap.c|   4 +-
 drivers/block/drbd/drbd_receiver.c  |  14 +---
 drivers/block/floppy.c  |   9 +--
 drivers/block/loop.c|   5 +-
 drivers/block/pktcdvd.c |   3 +-
 drivers/md/bcache/debug.c   |  11 ++-
 drivers/md/bcache/io.c  |   3 +-
 drivers/md/bcache/journal.c |   3 +-
 drivers/md/bcache/movinggc.c|   6 +-
 drivers/md/bcache/super.c   |  33 ++---
 drivers/md/bcache/writeback.c   |   4 +-
 drivers/md/dm-bufio.c   |   3 +-
 drivers/md/dm-crypt.c   |   8 +--
 drivers/md/dm-io.c  |   7 +-
 drivers/md/dm.c |   3 +-
 drivers/staging/lustre/lustre/llite/lloop.c |   9 +--
 drivers/target/target_core_pscsi.c  |   8 +--
 fs/logfs/dev_bdev.c | 107 +++-
 include/linux/bio.h |  21 ++
 kernel/power/swap.c |  10 ++-
 mm/page_io.c|  18 -
 21 files changed, 155 insertions(+), 134 deletions(-)

Thanks,
Ming
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[patch] scsi_dh_alua: uninitialized variable in alua_rtpg()

[Dan Carpenter]

It's possible to use "err" without initializing it.  If it happens to be
a 2 which is SCSI_DH_RETRY then that could cause a bug.

Signed-off-by: Dan Carpenter 

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c 
b/drivers/scsi/device_handler/scsi_dh_alua.c
index 8eaed05..f3c994f 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -513,7 +513,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct 
alua_port_group *pg)
struct alua_port_group *tmp_pg;
int len, k, off, valid_states = 0, bufflen = ALUA_RTPG_SIZE;
unsigned char *desc, *buff;
-   unsigned err, retval;
+   unsigned int err = 0;
+   unsigned int retval;
unsigned int tpg_desc_tbl_off;
unsigned char orig_transition_tmo;
unsigned long flags;
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[patch] bnx2i: silence uninitialized variable warnings

[Dan Carpenter]

Presumably it isn't possible to have empty lists here, but my static
checker doesn't know that and complains that "ep" can be used
uninitialized.

Signed-off-by: Dan Carpenter 

diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c
index 7289437..133901f 100644
--- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
+++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
@@ -675,7 +675,7 @@ bnx2i_find_ep_in_ofld_list(struct bnx2i_hba *hba, u32 
iscsi_cid)
 {
struct list_head *list;
struct list_head *tmp;
-   struct bnx2i_endpoint *ep;
+   struct bnx2i_endpoint *ep = NULL;
 
read_lock_bh(>ep_rdwr_lock);
list_for_each_safe(list, tmp, >ep_ofld_list) {
@@ -703,7 +703,7 @@ bnx2i_find_ep_in_destroy_list(struct bnx2i_hba *hba, u32 
iscsi_cid)
 {
struct list_head *list;
struct list_head *tmp;
-   struct bnx2i_endpoint *ep;
+   struct bnx2i_endpoint *ep = NULL;
 
read_lock_bh(>ep_rdwr_lock);
list_for_each_safe(list, tmp, >ep_destroy_list) {
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 03/14] libsas: Define ATA_CMD_NCQ_NON_DATA

[Hannes Reinecke]

On 04/14/2016 11:06 AM, John Garry wrote:
> On 12/04/2016 11:25, Hannes Reinecke wrote:
>> On 04/12/2016 12:03 PM, John Garry wrote:
>>> On 12/04/2016 07:47, Hannes Reinecke wrote:
 Define the NCQ NON DATA command and update libsas to handle it
 correctly.

 Signed-off-by: Hannes Reinecke 
 ---
drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 1 +
drivers/scsi/isci/request.c| 3 ++-
drivers/scsi/libsas/sas_ata.c  | 3 ++-
drivers/scsi/mvsas/mv_sas.c| 3 ++-
drivers/scsi/pm8001/pm8001_sas.c   | 3 ++-
include/linux/ata.h| 1 +
include/trace/events/libata.h  | 1 +
7 files changed, 11 insertions(+), 4 deletions(-)

 diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
 b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
 index fc2e767..ebaf5ab 100644
 --- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
 +++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
 @@ -1575,6 +1575,7 @@ static u8 get_ata_protocol(u8 cmd, int
 direction)
case ATA_CMD_FPDMA_READ:
case ATA_CMD_FPMDA_RECV:
case ATA_CMD_FPDMA_SEND:
 +case ATA_CMD_NCQ_NON_DATA:
return SATA_PROTOCOL_FPDMA;

>>>
>>> I'm going to double-check this. It may correspond to
>>> SATA_PROTOCOL_NONDATA, and not SATA_PROTOCOL_FPDMA.
>>>
> 
> So I got confirmation that SATA_PROTOCOL_FPDMA is correct for this
> type of command and we do support it.
> 
> If we want to test is it ok just to take this patchset and the other
> advised prerequisite patchsets?
> 
Yes, that should be sufficient.

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[patch] scsi: ufs: silence uninitialized variable warning

[Dan Carpenter]

If ufshcd_dme_get() fails then "tx_lanes" can be uninitialized.  I've
initialized it to zero so that the rest of the function turns into a
no-op in that situation.

Signed-off-by: Dan Carpenter 

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index f8fa72c..6741d9e 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3038,7 +3038,9 @@ static int ufshcd_hba_enable(struct ufs_hba *hba)
 
 static int ufshcd_disable_tx_lcc(struct ufs_hba *hba, bool peer)
 {
-   int tx_lanes, i, err = 0;
+   int tx_lanes = 0;
+   int err = 0;
+   int i;
 
if (!peer)
ufshcd_dme_get(hba, UIC_ARG_MIB(PA_CONNECTEDTXDATALANES),
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[patch] hpsa: set the enclosure identifier to zero

[Dan Carpenter]

This has only called from show_sas_rphy_enclosure_identifier().  The
caller expects that we set an identifier, otherwise it uses an
unintialized variable.

Signed-off-by: Dan Carpenter 

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 5be944c..25aa219 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -9602,6 +9602,7 @@ hpsa_sas_get_linkerrors(struct sas_phy *phy)
 static int
 hpsa_sas_get_enclosure_identifier(struct sas_rphy *rphy, u64 *identifier)
 {
+   *identifier = 0;
return 0;
 }
 
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[patch] target: Silence an uninitialized variable warning

[Dan Carpenter]

I'm getting a static checker warning:

drivers/target/target_core_sbc.c:1150 sbc_parse_cdb()
error: uninitialized variable 'size'.

It looks like a possible bug but wouldn't it have shown up in testing?
Anyway let's just silence it by setting size to zero.

Signed-off-by: Dan Carpenter 

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index a9057aa..0c6e55f 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -828,7 +828,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 {
struct se_device *dev = cmd->se_dev;
unsigned char *cdb = cmd->t_task_cdb;
-   unsigned int size;
+   unsigned int size = 0;
u32 sectors = 0;
sense_reason_t ret;
 
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 03/14] libsas: Define ATA_CMD_NCQ_NON_DATA

[John Garry]

On 12/04/2016 11:25, Hannes Reinecke wrote:

On 04/12/2016 12:03 PM, John Garry wrote:

On 12/04/2016 07:47, Hannes Reinecke wrote:

Define the NCQ NON DATA command and update libsas to handle it
correctly.

Signed-off-by: Hannes Reinecke 
---
   drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 1 +
   drivers/scsi/isci/request.c| 3 ++-
   drivers/scsi/libsas/sas_ata.c  | 3 ++-
   drivers/scsi/mvsas/mv_sas.c| 3 ++-
   drivers/scsi/pm8001/pm8001_sas.c   | 3 ++-
   include/linux/ata.h| 1 +
   include/trace/events/libata.h  | 1 +
   7 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
index fc2e767..ebaf5ab 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
@@ -1575,6 +1575,7 @@ static u8 get_ata_protocol(u8 cmd, int
direction)
   case ATA_CMD_FPDMA_READ:
   case ATA_CMD_FPMDA_RECV:
   case ATA_CMD_FPDMA_SEND:
+case ATA_CMD_NCQ_NON_DATA:
   return SATA_PROTOCOL_FPDMA;



I'm going to double-check this. It may correspond to
SATA_PROTOCOL_NONDATA, and not SATA_PROTOCOL_FPDMA.



So I got confirmation that SATA_PROTOCOL_FPDMA is correct for this type 
of command and we do support it.


If we want to test is it ok just to take this patchset and the other 
advised prerequisite patchsets?



Thanks. The spec is not exactly clear in that regard...

Cheers,

Hannes




--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] target: use new "dbroot" target attribute

[Hannes Reinecke]

On 04/13/2016 10:25 PM, Lee Duncan wrote:
> This commit updates the target core ALUA and PR
> modules to use the new "dbroot" attribute instead
> of assuming the target database is in "/var/target".
> 
> Signed-off-by: Lee Duncan 
> ---
>  drivers/target/target_core_alua.c | 6 +++---
>  drivers/target/target_core_pr.c   | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 1/2] target: make target db location configurable

[Hannes Reinecke]

On 04/13/2016 10:25 PM, Lee Duncan wrote:
> This commit adds the read-write attribute "dbroot",
> in the top-level CONFIGFS (core) target directory,
> normally /sys/kernel/config/target. This attribute
> defaults to "/var/target" but can be changed by
> writing a new pathname string to it. Changing this
> attribute is only allowed when no fabric drivers
> are loaded and the supplied value specifies an
> existing directory.
> 
> Target modules that care about the target database
> root directory will be modified to use this
> attribute in a future commit.
> 
> Signed-off-by: Lee Duncan 
> ---
>  drivers/target/target_core_configfs.c | 51 
> +++
>  drivers/target/target_core_internal.h |  6 +
>  2 files changed, 57 insertions(+)
> 
> diff --git a/drivers/target/target_core_configfs.c 
> b/drivers/target/target_core_configfs.c
> index 713c63d9681b..bfedbd92b77f 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -99,6 +99,56 @@ static ssize_t target_core_item_version_show(struct 
> config_item *item,
>  
>  CONFIGFS_ATTR_RO(target_core_item_, version);
>  
> +char db_root[DB_ROOT_LEN] = DB_ROOT_DEFAULT;
> +static char db_root_stage[DB_ROOT_LEN];
> +
> +static ssize_t target_core_item_dbroot_show(struct config_item *item,
> + char *page)
> +{
> + return sprintf(page, "%s\n", db_root);
> +}
> +
> +static ssize_t target_core_item_dbroot_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + ssize_t read_bytes;
> + struct file *fp;
> +
> + if (!list_empty(_tf_list)) {
> + pr_err("db_root: cannot be changed: target drivers registered");
> + return -EINVAL;
> + }
Locking?

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/42] v5: separate operations from flags in the bio/request structs

[Hannes Reinecke]

On 04/13/2016 09:35 PM, mchri...@redhat.com wrote:
> The following patches begin to cleanup the request->cmd_flags and
> bio->bi_rw mess. We currently use cmd_flags to specify the operation,
> attributes and state of the request. For bi_rw we use it for similar
> info and also the priority but then also have another bi_flags field
> for state. At some point, we abused them so much we just made cmd_flags
> 64 bits, so we could add more.
> 
> The following patches seperate the operation (read, write discard,
> flush, etc) from cmd_flags/bi_rw.
> 
> This patchset was made against linux-next from today April 13
> (git tag next-20160413).
> 
> I put a git tree here:
> https://github.com/mikechristie/linux-kernel.git
> The patches are in the op branch.
> 
A round of applause for you.

For the entire series:

Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html