RE: [PATCH 0/5] blk-mq/scsi-mq: support global tags & introduce force_blk_mq

2018-02-04 Thread Kashyap Desai 
> -Original Message-
> From: Hannes Reinecke [mailto:h...@suse.de]
> Sent: Monday, February 5, 2018 12:28 PM
> To: Ming Lei; Jens Axboe; linux-bl...@vger.kernel.org; Christoph Hellwig;
> Mike Snitzer
> Cc: linux-scsi@vger.kernel.org; Arun Easi; Omar Sandoval; Martin K .
> Petersen;
> James Bottomley; Christoph Hellwig; Don Brace; Kashyap Desai; Peter
> Rivera;
> Paolo Bonzini; Laurence Oberman
> Subject: Re: [PATCH 0/5] blk-mq/scsi-mq: support global tags & introduce
> force_blk_mq
>
> On 02/03/2018 05:21 AM, Ming Lei wrote:
> > Hi All,
> >
> > This patchset supports global tags which was started by Hannes
> > originally:
> >
> > https://marc.info/?l=linux-block=149132580511346=2
> >
> > Also inroduce 'force_blk_mq' to 'struct scsi_host_template', so that
> > driver can avoid to support two IO paths(legacy and blk-mq),
> > especially recent discusion mentioned that SCSI_MQ will be enabled at
> default soon.
> >
> > https://marc.info/?l=linux-scsi=151727684915589=2
> >
> > With the above two changes, it should be easier to convert SCSI drivers'
> > reply queue into blk-mq's hctx, then the automatic irq affinity issue
> > can be solved easily, please see detailed descrption in commit log.
> >
> > Also drivers may require to complete request on the submission CPU for
> > avoiding hard/soft deadlock, which can be done easily with blk_mq too.
> >
> > https://marc.info/?t=15160185141=1=2
> >
> > The final patch uses the introduced 'force_blk_mq' to fix virtio_scsi
> > so that IO hang issue can be avoided inside legacy IO path, this issue
> > is a bit generic, at least HPSA/virtio-scsi are found broken with
> > v4.15+.
> >
> > Thanks
> > Ming
> >
> > Ming Lei (5):
> >   blk-mq: tags: define several fields of tags as pointer
> >   blk-mq: introduce BLK_MQ_F_GLOBAL_TAGS
> >   block: null_blk: introduce module parameter of 'g_global_tags'
> >   scsi: introduce force_blk_mq
> >   scsi: virtio_scsi: fix IO hang by irq vector automatic affinity
> >
> >  block/bfq-iosched.c|  4 +--
> >  block/blk-mq-debugfs.c | 11 
> >  block/blk-mq-sched.c   |  2 +-
> >  block/blk-mq-tag.c | 67
> > +-
> >  block/blk-mq-tag.h | 15 ---
> >  block/blk-mq.c | 31 -
> >  block/blk-mq.h |  3 ++-
> >  block/kyber-iosched.c  |  2 +-
> >  drivers/block/null_blk.c   |  6 +
> >  drivers/scsi/hosts.c   |  1 +
> >  drivers/scsi/virtio_scsi.c | 59
> > +++-
> >  include/linux/blk-mq.h |  2 ++
> >  include/scsi/scsi_host.h   |  3 +++
> >  13 files changed, 105 insertions(+), 101 deletions(-)
> >
> Thanks Ming for picking this up.
>
> I'll give it a shot and see how it behaves on other hardware.

Ming -

There is no way we can enable global tags from SCSI stack in this patch
series.   I still think we have no solution for issue described below in
this patch series.
https://marc.info/?t=15160185141=1=2

What we will be doing is just use global tag HBA wide instead of h/w queue
based. We still have more than one reply queue ending up completion one CPU.
Try to reduce MSI-x vector of megaraid_sas or mpt3sas driver via module
parameter to simulate the issue. We need more number of Online CPU than
reply-queue.
We may see completion redirected to original CPU because of
"QUEUE_FLAG_SAME_FORCE", but ISR of low level driver can keep one CPU busy
in local ISR routine.


Kashyap

>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke  Teamlead 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)

Re: [PATCH 0/5] blk-mq/scsi-mq: support global tags & introduce force_blk_mq

2018-02-04 Thread Hannes Reinecke 
On 02/03/2018 05:21 AM, Ming Lei wrote:
> Hi All,
> 
> This patchset supports global tags which was started by Hannes originally:
> 
>   https://marc.info/?l=linux-block=149132580511346=2
> 
> Also inroduce 'force_blk_mq' to 'struct scsi_host_template', so that
> driver can avoid to support two IO paths(legacy and blk-mq), especially
> recent discusion mentioned that SCSI_MQ will be enabled at default soon.
> 
>   https://marc.info/?l=linux-scsi=151727684915589=2
> 
> With the above two changes, it should be easier to convert SCSI drivers'
> reply queue into blk-mq's hctx, then the automatic irq affinity issue can
> be solved easily, please see detailed descrption in commit log.
> 
> Also drivers may require to complete request on the submission CPU
> for avoiding hard/soft deadlock, which can be done easily with blk_mq
> too.
> 
>   https://marc.info/?t=15160185141=1=2
> 
> The final patch uses the introduced 'force_blk_mq' to fix virtio_scsi
> so that IO hang issue can be avoided inside legacy IO path, this issue is
> a bit generic, at least HPSA/virtio-scsi are found broken with v4.15+.
> 
> Thanks
> Ming
> 
> Ming Lei (5):
>   blk-mq: tags: define several fields of tags as pointer
>   blk-mq: introduce BLK_MQ_F_GLOBAL_TAGS
>   block: null_blk: introduce module parameter of 'g_global_tags'
>   scsi: introduce force_blk_mq
>   scsi: virtio_scsi: fix IO hang by irq vector automatic affinity
> 
>  block/bfq-iosched.c|  4 +--
>  block/blk-mq-debugfs.c | 11 
>  block/blk-mq-sched.c   |  2 +-
>  block/blk-mq-tag.c | 67 
> +-
>  block/blk-mq-tag.h | 15 ---
>  block/blk-mq.c | 31 -
>  block/blk-mq.h |  3 ++-
>  block/kyber-iosched.c  |  2 +-
>  drivers/block/null_blk.c   |  6 +
>  drivers/scsi/hosts.c   |  1 +
>  drivers/scsi/virtio_scsi.c | 59 +++-
>  include/linux/blk-mq.h |  2 ++
>  include/scsi/scsi_host.h   |  3 +++
>  13 files changed, 105 insertions(+), 101 deletions(-)
> 
Thanks Ming for picking this up.

I'll give it a shot and see how it behaves on other hardware.

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)

Re: [PATCH 1/5] blk-mq: tags: define several fields of tags as pointer

2018-02-04 Thread Hannes Reinecke 
On 02/03/2018 05:21 AM, Ming Lei wrote:
> This patch changes tags->breserved_tags, tags->bitmap_tags and
> tags->active_queues as pointer, and prepares for supporting global tags.
> 
> No functional change.
> 
> Cc: Laurence Oberman 
> Cc: Mike Snitzer 
> Cc: Christoph Hellwig 
> Signed-off-by: Ming Lei 
> ---
>  block/bfq-iosched.c|  4 ++--
>  block/blk-mq-debugfs.c | 10 +-
>  block/blk-mq-tag.c | 48 ++--
>  block/blk-mq-tag.h | 10 +++---
>  block/blk-mq.c |  2 +-
>  block/kyber-iosched.c  |  2 +-
>  6 files changed, 42 insertions(+), 34 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)

Re: [PATCH 5/5] scsi: virtio_scsi: fix IO hang by irq vector automatic affinity

2018-02-04 Thread Hannes Reinecke 
On 02/03/2018 05:21 AM, Ming Lei wrote:
> Now 84676c1f21e8ff5(genirq/affinity: assign vectors to all possible CPUs)
> has been merged to V4.16-rc, and it is easy to allocate all offline CPUs
> for some irq vectors, this can't be avoided even though the allocation
> is improved.
> 
> For example, on a 8cores VM, 4~7 are not-present/offline, 4 queues of
> virtio-scsi, the irq affinity assigned can become the following shape:
> 
>   irq 36, cpu list 0-7
>   irq 37, cpu list 0-7
>   irq 38, cpu list 0-7
>   irq 39, cpu list 0-1
>   irq 40, cpu list 4,6
>   irq 41, cpu list 2-3
>   irq 42, cpu list 5,7
> 
> Then IO hang is triggered in case of non-SCSI_MQ.
> 
> Given storage IO is always C/S model, there isn't such issue with 
> SCSI_MQ(blk-mq),
> because no IO can be submitted to one hw queue if the hw queue hasn't online
> CPUs.
> 
> Fix this issue by forcing to use blk_mq.
> 
> BTW, I have been used virtio-scsi(scsi_mq) for several years, and it has
> been quite stable, so it shouldn't cause extra risk.
> 
> Cc: Hannes Reinecke 
> Cc: Arun Easi 
> Cc: Omar Sandoval ,
> Cc: "Martin K. Petersen" ,
> Cc: James Bottomley ,
> Cc: Christoph Hellwig ,
> Cc: Don Brace 
> Cc: Kashyap Desai 
> Cc: Peter Rivera 
> Cc: Paolo Bonzini 
> Cc: Laurence Oberman 
> Cc: Mike Snitzer 
> Signed-off-by: Ming Lei 
> ---
>  drivers/scsi/virtio_scsi.c | 59 
> +++---
>  1 file changed, 3 insertions(+), 56 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)

Re: [PATCH 4/5] scsi: introduce force_blk_mq

2018-02-04 Thread Hannes Reinecke 
On 02/03/2018 05:21 AM, Ming Lei wrote:
> From scsi driver view, it is a bit troublesome to support both blk-mq
> and non-blk-mq at the same time, especially when drivers need to support
> multi hw-queue.
> 
> This patch introduces 'force_blk_mq' to scsi_host_template so that drivers
> can provide blk-mq only support, so driver code can avoid the trouble
> for supporting both.
> 
> This patch may clean up driver a lot by providing blk-mq only support, 
> espeically
> it is easier to convert multiple reply queues into blk_mq's MQ for the 
> following
> purposes:
> 
> 1) use blk_mq multiple hw queue to deal with allocated irq vectors of all 
> offline
> CPU affinity[1]:
> 
>   [1] https://marc.info/?l=linux-kernel=151748144730409=2
> 
> Now 84676c1f21e8ff5(genirq/affinity: assign vectors to all possible CPUs)
> has been merged to V4.16-rc, and it is easy to allocate all offline CPUs
> for some irq vectors, this can't be avoided even though the allocation
> is improved.
> 
> So all these drivers have to avoid to ask HBA to complete request in
> reply queue which hasn't online CPUs assigned.
> 
> This issue can be solved generically and easily via blk_mq(scsi_mq) multiple
> hw queue by mapping each reply queue into hctx.
> 
> 2) some drivers[1] require to complete request in the submission CPU for
> avoiding hard/soft lockup, which is easily done with blk_mq, so not necessary
> to reinvent wheels for solving the problem.
> 
>   [2] https://marc.info/?t=15160185141=1=2
> 
> Sovling the above issues for non-MQ path may not be easy, or introduce
> unnecessary work, especially we plan to enable SCSI_MQ soon as discussed
> recently[3]:
> 
>   [3] https://marc.info/?l=linux-scsi=151727684915589=2
> 
> Cc: Hannes Reinecke 
> Cc: Arun Easi 
> Cc: Omar Sandoval ,
> Cc: "Martin K. Petersen" ,
> Cc: James Bottomley ,
> Cc: Christoph Hellwig ,
> Cc: Don Brace 
> Cc: Kashyap Desai 
> Cc: Peter Rivera 
> Cc: Laurence Oberman 
> Cc: Mike Snitzer 
> Signed-off-by: Ming Lei 
> ---
>  drivers/scsi/hosts.c | 1 +
>  include/scsi/scsi_host.h | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index fe3a0da3ec97..c75cebd7911d 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -471,6 +471,7 @@ struct Scsi_Host *scsi_host_alloc(struct 
> scsi_host_template *sht, int privsize)
>   shost->dma_boundary = 0x;
>  
>   shost->use_blk_mq = scsi_use_blk_mq;
> + shost->use_blk_mq = scsi_use_blk_mq || !!shost->hostt->force_blk_mq;
>  
>   device_initialize(>shost_gendev);
>   dev_set_name(>shost_gendev, "host%d", shost->host_no);
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index a8b7bf879ced..4118760e5c32 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -452,6 +452,9 @@ struct scsi_host_template {
>   /* True if the controller does not support WRITE SAME */
>   unsigned no_write_same:1;
>  
> + /* tell scsi core we support blk-mq only */
> + unsigned force_blk_mq:1;
> +
>   /*
>* Countdown for host blocking with no commands outstanding.
>*/
> 
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)

Re: [PATCH 2/5] blk-mq: introduce BLK_MQ_F_GLOBAL_TAGS

2018-02-04 Thread Hannes Reinecke 
On 02/03/2018 05:21 AM, Ming Lei wrote:
> Quite a few HBAs(such as HPSA, megaraid, mpt3sas, ..) support multiple
> reply queues, but tags is often HBA wide.
> 
> These HBAs have switched to use pci_alloc_irq_vectors(PCI_IRQ_AFFINITY)
> for automatic affinity assignment.
> 
> Now 84676c1f21e8ff5(genirq/affinity: assign vectors to all possible CPUs)
> has been merged to V4.16-rc, and it is easy to allocate all offline CPUs
> for some irq vectors, this can't be avoided even though the allocation
> is improved.
> 
> So all these drivers have to avoid to ask HBA to complete request in
> reply queue which hasn't online CPUs assigned, and HPSA has been broken
> with v4.15+:
> 
>   https://marc.info/?l=linux-kernel=151748144730409=2
> 
> This issue can be solved generically and easily via blk_mq(scsi_mq) multiple
> hw queue by mapping each reply queue into hctx, but one tricky thing is
> the HBA wide(instead of hw queue wide) tags.
> 
> This patch is based on the following Hannes's patch:
> 
>   https://marc.info/?l=linux-block=149132580511346=2
> 
> One big difference with Hannes's is that this patch only makes the tags 
> sbitmap
> and active_queues data structure HBA wide, and others are kept as NUMA 
> locality,
> such as request, hctx, tags, ...
> 
> The following patch will support global tags on null_blk, also the performance
> data is provided, no obvious performance loss is observed when the whole
> hw queue depth is same.
> 
> Cc: Hannes Reinecke 
> Cc: Arun Easi 
> Cc: Omar Sandoval ,
> Cc: "Martin K. Petersen" ,
> Cc: James Bottomley ,
> Cc: Christoph Hellwig ,
> Cc: Don Brace 
> Cc: Kashyap Desai 
> Cc: Peter Rivera 
> Cc: Laurence Oberman 
> Cc: Mike Snitzer 
> Signed-off-by: Ming Lei 
> ---
>  block/blk-mq-debugfs.c |  1 +
>  block/blk-mq-sched.c   |  2 +-
>  block/blk-mq-tag.c | 23 ++-
>  block/blk-mq-tag.h |  5 -
>  block/blk-mq.c | 29 -
>  block/blk-mq.h |  3 ++-
>  include/linux/blk-mq.h |  2 ++
>  7 files changed, 52 insertions(+), 13 deletions(-)
> 
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index 0dfafa4b655a..0f0fafe03f5d 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -206,6 +206,7 @@ static const char *const hctx_flag_name[] = {
>   HCTX_FLAG_NAME(SHOULD_MERGE),
>   HCTX_FLAG_NAME(TAG_SHARED),
>   HCTX_FLAG_NAME(SG_MERGE),
> + HCTX_FLAG_NAME(GLOBAL_TAGS),
>   HCTX_FLAG_NAME(BLOCKING),
>   HCTX_FLAG_NAME(NO_SCHED),
>  };
> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> index 55c0a745b427..191d4bc95f0e 100644
> --- a/block/blk-mq-sched.c
> +++ b/block/blk-mq-sched.c
> @@ -495,7 +495,7 @@ static int blk_mq_sched_alloc_tags(struct request_queue 
> *q,
>   int ret;
>  
>   hctx->sched_tags = blk_mq_alloc_rq_map(set, hctx_idx, q->nr_requests,
> -set->reserved_tags);
> +set->reserved_tags, false);
>   if (!hctx->sched_tags)
>   return -ENOMEM;
>  
> diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
> index 571797dc36cb..66377d09eaeb 100644
> --- a/block/blk-mq-tag.c
> +++ b/block/blk-mq-tag.c
> @@ -379,9 +379,11 @@ static struct blk_mq_tags 
> *blk_mq_init_bitmap_tags(struct blk_mq_tags *tags,
>   return NULL;
>  }
>  
> -struct blk_mq_tags *blk_mq_init_tags(unsigned int total_tags,
> +struct blk_mq_tags *blk_mq_init_tags(struct blk_mq_tag_set *set,
> +  unsigned int total_tags,
>unsigned int reserved_tags,
> -  int node, int alloc_policy)
> +  int node, int alloc_policy,
> +  bool global_tag)
>  {
>   struct blk_mq_tags *tags;
>  
> @@ -397,6 +399,14 @@ struct blk_mq_tags *blk_mq_init_tags(unsigned int 
> total_tags,
>   tags->nr_tags = total_tags;
>   tags->nr_reserved_tags = reserved_tags;
>  
> + WARN_ON(global_tag && !set->global_tags);
> + if (global_tag && set->global_tags) {
> + tags->bitmap_tags = set->global_tags->bitmap_tags;
> + tags->breserved_tags = set->global_tags->breserved_tags;
> + tags->active_queues = set->global_tags->active_queues;
> + tags->global_tags = true;
> + return tags;
> + }
>   tags->bitmap_tags = >__bitmap_tags;
>   tags->breserved_tags = >__breserved_tags;
>   tags->active_queues = >__active_queues;
Do we really need the 'global_tag' flag here?
Can't we just rely on the ->global_tags pointer to be set?

> @@ -406,8 +416,10 @@ struct blk_mq_tags

Re: [PATCH 3/5] block: null_blk: introduce module parameter of 'g_global_tags'

2018-02-04 Thread Hannes Reinecke 
On 02/03/2018 05:21 AM, Ming Lei wrote:
> This patch introduces the parameter of 'g_global_tags' so that we can
> test this feature by null_blk easiy.
> 
> Not see obvious performance drop with global_tags when the whole hw
> depth is kept as same:
> 
> 1) no 'global_tags', each hw queue depth is 1, and 4 hw queues
> modprobe null_blk queue_mode=2 nr_devices=4 shared_tags=1 global_tags=0 
> submit_queues=4 hw_queue_depth=1
> 
> 2) 'global_tags', global hw queue depth is 4, and 4 hw queues
> modprobe null_blk queue_mode=2 nr_devices=4 shared_tags=1 global_tags=0 
> submit_queues=4 hw_queue_depth=4
> 
> 3) fio test done in above two settings:
>fio --bs=4k --size=512G  --rw=randread --norandommap --direct=1 
> --ioengine=libaio --iodepth=4 --runtime=$RUNTIME --group_reporting=1  
> --name=nullb0 --filename=/dev/nullb0 --name=nullb1 --filename=/dev/nullb1 
> --name=nullb2 --filename=/dev/nullb2 --name=nullb3 --filename=/dev/nullb3
> 
> 1M IOPS can be reached in both above tests which is done in one VM.
> 
> Cc: Hannes Reinecke 
> Cc: Arun Easi 
> Cc: Omar Sandoval ,
> Cc: "Martin K. Petersen" ,
> Cc: James Bottomley ,
> Cc: Christoph Hellwig ,
> Cc: Don Brace 
> Cc: Kashyap Desai 
> Cc: Peter Rivera 
> Cc: Laurence Oberman 
> Cc: Mike Snitzer 
> Signed-off-by: Ming Lei 
> ---
>  drivers/block/null_blk.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/block/null_blk.c b/drivers/block/null_blk.c
> index 287a09611c0f..ad0834efad42 100644
> --- a/drivers/block/null_blk.c
> +++ b/drivers/block/null_blk.c
> @@ -163,6 +163,10 @@ static int g_submit_queues = 1;
>  module_param_named(submit_queues, g_submit_queues, int, S_IRUGO);
>  MODULE_PARM_DESC(submit_queues, "Number of submission queues");
>  
> +static int g_global_tags = 0;
> +module_param_named(global_tags, g_global_tags, int, S_IRUGO);
> +MODULE_PARM_DESC(global_tags, "All submission queues share one tags");
> +
>  static int g_home_node = NUMA_NO_NODE;
>  module_param_named(home_node, g_home_node, int, S_IRUGO);
>  MODULE_PARM_DESC(home_node, "Home node for the device");
> @@ -1622,6 +1626,8 @@ static int null_init_tag_set(struct nullb *nullb, 
> struct blk_mq_tag_set *set)
>   set->flags = BLK_MQ_F_SHOULD_MERGE;
>   if (g_no_sched)
>   set->flags |= BLK_MQ_F_NO_SCHED;
> + if (g_global_tags)
> + set->flags |= BLK_MQ_F_GLOBAL_TAGS;
>   set->driver_data = NULL;
>  
>   if ((nullb && nullb->dev->blocking) || g_blocking)
> 
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)

Re: [PATCH 5/6] scsi: qedi: fix building with LTO

2018-02-04 Thread Rangankar, Manish 


On 02/02/18 6:42 PM, "Arnd Bergmann"  wrote:

>When link-time optimizations are enabled, qedi fails to build because
>of mismatched prototypes:
>
>drivers/scsi/qedi/qedi_gbl.h:27:37: error: type of 'qedi_dbg_fops' does
>not match original declaration [-Werror=lto-type-mismatch]
> extern const struct file_operations qedi_dbg_fops;
> ^
>drivers/scsi/qedi/qedi_debugfs.c:239:30: note: 'qedi_dbg_fops' was
>previously declared here
> const struct file_operations qedi_dbg_fops[] = {
>  ^
>drivers/scsi/qedi/qedi_gbl.h:26:32: error: type of 'qedi_debugfs_ops'
>does not match original declaration [-Werror=lto-type-mismatch]
> extern struct qedi_debugfs_ops qedi_debugfs_ops;
>^
>drivers/scsi/qedi/qedi_debugfs.c:102:25: note: 'qedi_debugfs_ops' was
>previously declared here
> struct qedi_debugfs_ops qedi_debugfs_ops[] = {
>
>This changes the declaration to match the definition, and adapts the
>users as necessary. Since both array can be constant here, I'm adding
>the 'const' everywhere for consistency.
>
>Signed-off-by: Arnd Bergmann 
>---
> drivers/scsi/qedi/qedi_dbg.h | 2 +-
> drivers/scsi/qedi/qedi_debugfs.c | 4 ++--
> drivers/scsi/qedi/qedi_gbl.h | 4 ++--
> drivers/scsi/qedi/qedi_main.c| 4 ++--
> 4 files changed, 7 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/scsi/qedi/qedi_dbg.h b/drivers/scsi/qedi/qedi_dbg.h
>index c55572badfb0..358f40567849 100644
>--- a/drivers/scsi/qedi/qedi_dbg.h
>+++ b/drivers/scsi/qedi/qedi_dbg.h
>@@ -134,7 +134,7 @@ struct qedi_debugfs_ops {
> }
> 
> void qedi_dbg_host_init(struct qedi_dbg_ctx *qedi,
>-  struct qedi_debugfs_ops *dops,
>+  const struct qedi_debugfs_ops *dops,
>   const struct file_operations *fops);
> void qedi_dbg_host_exit(struct qedi_dbg_ctx *qedi);
> void qedi_dbg_init(char *drv_name);
>diff --git a/drivers/scsi/qedi/qedi_debugfs.c
>b/drivers/scsi/qedi/qedi_debugfs.c
>index fd8a1eea3163..fd914ca4149a 100644
>--- a/drivers/scsi/qedi/qedi_debugfs.c
>+++ b/drivers/scsi/qedi/qedi_debugfs.c
>@@ -19,7 +19,7 @@ static struct dentry *qedi_dbg_root;
> 
> void
> qedi_dbg_host_init(struct qedi_dbg_ctx *qedi,
>- struct qedi_debugfs_ops *dops,
>+ const struct qedi_debugfs_ops *dops,
>  const struct file_operations *fops)
> {
>   char host_dirname[32];
>@@ -99,7 +99,7 @@ static struct qedi_list_of_funcs
>qedi_dbg_do_not_recover_ops[] = {
>   { NULL, NULL }
> };
> 
>-struct qedi_debugfs_ops qedi_debugfs_ops[] = {
>+const struct qedi_debugfs_ops qedi_debugfs_ops[] = {
>   { "gbl_ctx", NULL },
>   { "do_not_recover", qedi_dbg_do_not_recover_ops},
>   { "io_trace", NULL },
>diff --git a/drivers/scsi/qedi/qedi_gbl.h b/drivers/scsi/qedi/qedi_gbl.h
>index f5b5a31999aa..a2aa06ed1620 100644
>--- a/drivers/scsi/qedi/qedi_gbl.h
>+++ b/drivers/scsi/qedi/qedi_gbl.h
>@@ -23,8 +23,8 @@ extern uint qedi_io_tracing;
> extern struct scsi_host_template qedi_host_template;
> extern struct iscsi_transport qedi_iscsi_transport;
> extern const struct qed_iscsi_ops *qedi_ops;
>-extern struct qedi_debugfs_ops qedi_debugfs_ops;
>-extern const struct file_operations qedi_dbg_fops;
>+extern const struct qedi_debugfs_ops qedi_debugfs_ops[];
>+extern const struct file_operations qedi_dbg_fops[];
> extern struct device_attribute *qedi_shost_attrs[];
> 
> int qedi_alloc_sq(struct qedi_ctx *qedi, struct qedi_endpoint *ep);
>diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
>index 029e2e69b29f..e992f9d3ef00 100644
>--- a/drivers/scsi/qedi/qedi_main.c
>+++ b/drivers/scsi/qedi/qedi_main.c
>@@ -2303,8 +2303,8 @@ static int __qedi_probe(struct pci_dev *pdev, int
>mode)
>   }
> 
> #ifdef CONFIG_DEBUG_FS
>-  qedi_dbg_host_init(>dbg_ctx, _debugfs_ops,
>- _dbg_fops);
>+  qedi_dbg_host_init(>dbg_ctx, qedi_debugfs_ops,
>+ qedi_dbg_fops);
> #endif
>   QEDI_INFO(>dbg_ctx, QEDI_LOG_INFO,
> "QLogic FastLinQ iSCSI Module qedi %s, FW %d.%d.%d.%d\n",
>-- 
>2.9.0

Thanks

Acked-by: Manish Rangankar 


>

Re: [PATCH 1/1] scsi: ufs: make sure all interrupts are processed

2018-02-04 Thread Asutosh Das (asd) 

On 2/2/2018 8:53 AM, Asutosh Das (asd) wrote:

On 1/31/2018 1:09 PM, Avri Altman wrote:

Hi,
Can you elaborate how this can even happen?
Isn't the interrupt aggregation capability should attend for those cases?

Thanks,
Avri


-Original Message-
From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
ow...@vger.kernel.org] On Behalf Of Asutosh Das
Sent: Tuesday, January 30, 2018 6:54 AM
To: subha...@codeaurora.org; c...@codeaurora.org;
vivek.gau...@codeaurora.org; rna...@codeaurora.org;
vinholika...@gmail.com; j...@linux.vnet.ibm.com;
martin.peter...@oracle.com
Cc: linux-scsi@vger.kernel.org; Venkat Gopalakrishnan
; Asutosh Das ; open
list 
Subject: [PATCH 1/1] scsi: ufs: make sure all interrupts are processed

From: Venkat Gopalakrishnan 

As multiple requests are submitted to the ufs host controller in 
parallel there
could be instances where the command completion interrupt arrives 
later for a
request that is already processed earlier as the corresponding 
doorbell was
cleared when handling the previous interrupt. Read the interrupt 
status in a
loop after processing the received interrupt to catch such interrupts 
and handle

it.

Signed-off-by: Venkat Gopalakrishnan 
Signed-off-by: Asutosh Das 
---
  drivers/scsi/ufs/ufshcd.c | 27 +++
  1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
8af2af3..58d81de 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5357,19 +5357,30 @@ static irqreturn_t ufshcd_intr(int irq, void 
*__hba)

  u32 intr_status, enabled_intr_status;
  irqreturn_t retval = IRQ_NONE;
  struct ufs_hba *hba = __hba;
+    int retries = hba->nutrs;

  spin_lock(hba->host->host_lock);
  intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
-    enabled_intr_status =
-    intr_status & ufshcd_readl(hba, REG_INTERRUPT_ENABLE);

-    if (intr_status)
-    ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
+    /*
+ * There could be max of hba->nutrs reqs in flight and in worst 
case

+ * if the reqs get finished 1 by 1 after the interrupt status is
+ * read, make sure we handle them by checking the interrupt status
+ * again in a loop until we process all of the reqs before 
returning.

+ */
+    do {
+    enabled_intr_status =
+    intr_status & ufshcd_readl(hba,
REG_INTERRUPT_ENABLE);
+    if (intr_status)
+    ufshcd_writel(hba, intr_status,
REG_INTERRUPT_STATUS);
+    if (enabled_intr_status) {
+    ufshcd_sl_intr(hba, enabled_intr_status);
+    retval = IRQ_HANDLED;
+    }
+
+    intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
+    } while (intr_status && --retries);

-    if (enabled_intr_status) {
-    ufshcd_sl_intr(hba, enabled_intr_status);
-    retval = IRQ_HANDLED;
-    }
  spin_unlock(hba->host->host_lock);
  return retval;
  }
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux

Foundation Collaborative Project.




Hi
yes - interrupt aggregation makes sense here. But there were some 
performance concerns with it; well, I don't have the data to back that 
up now though.

However, I can code it up and check it.
Will post it in some time.

-asd


Hi Avri,
I went through the UFS HCI - v2.1 spec. Specifically, in sec 7.2.3 it 
explicitly mentions that the software should determine if new TRs were 
completed since the interrupt status was last read/cleared. This step is 
independent of aggregation.


So I think the above implementation makes sense. Please let me know if I 
understood your concern correctly.


-asd

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project

Re: [PATCH 6/6] scsi: qedf: use correct strncpy() size

2018-02-04 Thread kbuild test robot 
Hi Arnd,

I love your patch! Perhaps something to improve:

[auto build test WARNING on scsi/for-next]
[also build test WARNING on v4.15]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Arnd-Bergmann/scsi-fixes-for-building-with-LTO/20180205-083607
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: xtensa-allmodconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 7.2.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=xtensa 

All warnings (new ones prefixed by >>):

   drivers/scsi//qedf/qedf_dbg.c: In function 'qedf_uevent_emit':
>> drivers/scsi//qedf/qedf_dbg.c:163:4: warning: ignoring return value of 
>> 'strscpy', declared with attribute warn_unused_result [-Wunused-result]
   strscpy(event_string, msg, sizeof(event_string));
   ^~~~

vim +/strscpy +163 drivers/scsi//qedf/qedf_dbg.c

   152  
   153  void
   154  qedf_uevent_emit(struct Scsi_Host *shost, u32 code, char *msg)
   155  {
   156  char event_string[40];
   157  char *envp[] = {event_string, NULL};
   158  
   159  memset(event_string, 0, sizeof(event_string));
   160  switch (code) {
   161  case QEDF_UEVENT_CODE_GRCDUMP:
   162  if (msg)
 > 163  strscpy(event_string, msg, 
 > sizeof(event_string));
   164  else
   165  sprintf(event_string, "GRCDUMP=%u", 
shost->host_no);
   166  break;
   167  default:
   168  /* do nothing */
   169  break;
   170  }
   171  
   172  kobject_uevent_env(>shost_gendev.kobj, KOBJ_CHANGE, 
envp);
   173  }
   174  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

[Solved] Re: SSD being put to sleep during boot-up when laptop on battery

2018-02-04 Thread Adam Bennett 

The problem was resolved upon upgrading laptop-mode-tools to 1.72.2.

Adam


On 12/25/2017 11:29 AM, Alan Stern wrote:

On Sun, 24 Dec 2017, Adam Bennett wrote:


Alan,

I have a Dell Precision 7520 with an Samsung SSD in addition to an NVMe
drive.  I'm currently running 4.14.8 but have seen the below problem
since I moved from 4.9 to 4.13.

When I boot without AC plugged in, the SSD is stopped almost immediately
in the boot process, not allowing the boot to continue.

I can boot on fine on AC.  When I unplug the cord, I see the "Stopping
disk" message, and the system is unresponsive until I plug the AC back in.

I initially thought the problem was in user-space, but I set up
laptop-mode-tools to keep "control" as "on", and the problem still
persists.  Also, I don't see the issue in 4.9.

It's entirely possible that the problem does lie in userspace, during
boot-up.  Don't forget that your initramfs image could be causing this;
have you tried to rebuild it with the new laptop settings?


I have temporarily worked around the problem by returning from
sd_suspend_runtime without calling sd_suspend_common in sd.c (obviously
not the true fix).

I have tried a number of searches, and couldn't find any bug reports of
this nature, but I'm not that in-tune with the linux development process
to have searched all the correct places.

This should be sent to the linux-pm and linux-scsi mailing lists
(CC'ed).


I'd like to help track this problem down, do you have any suggestions or
is there some additional details I could provide?

You can try bisecting between the 4.9 and 4.13 kernels to find the
commit which first caused the problem.

Alan Stern

Re: [PATCH v4 01/10] ufs: sysfs: attribute group for existing sysfs entries.

2018-02-04 Thread Greg KH 
On Sun, Feb 04, 2018 at 01:09:09PM +, Stanislav Nijnikov wrote:
> 
> 
> > -Original Message-
> > From: Greg KH [mailto:gre...@linuxfoundation.org]
> > Sent: Sunday, February 4, 2018 2:34 PM
> > To: Stanislav Nijnikov 
> > Cc: linux-scsi@vger.kernel.org; linux-ker...@vger.kernel.org;
> > jaeg...@kernel.org; Alex Lemberg 
> > Subject: Re: [PATCH v4 01/10] ufs: sysfs: attribute group for existing sysfs
> > entries.
> > 
> > On Sun, Feb 04, 2018 at 12:29:06PM +, Stanislav Nijnikov wrote:
> > > > > + curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len),
> > > > > +  "\nAll available Runtime PM levels 
> > > > > info:\n");
> > > > > + for (lvl = UFS_PM_LVL_0; lvl < UFS_PM_LVL_MAX; lvl++)
> > > > > + curr_len += snprintf((buf + curr_len), (PAGE_SIZE - 
> > > > > curr_len),
> > > > > +  "\tRuntime PM level [%d] => 
> > > > > dev_state
> > > > [%s] link_state [%s]\n",
> > > > > + lvl,
> > > > > + ufschd_ufs_dev_pwr_mode_to_string(
> > > > > + 
> > > > > ufs_pm_lvl_states[lvl].dev_state),
> > > > > + ufschd_uic_link_state_to_string(
> > > > > + 
> > > > > ufs_pm_lvl_states[lvl].link_state));
> > > > > +
> > > >
> > > > sysfs if "one value per file", not "random text that someone has to
> > > > parse per file" please.
> > > >
> > > > Huge hint, if you ever care about checking the size of the sysfs
> > > > buffer you are writing into, you are doing something really really 
> > > > wrong.
> > > >
> > > Hi Greg
> > > It's the existing code, added by:
> > > commit 09690d5a6ae1b7e4cb5ac429c311b99d09352c12
> > > Author: subha...@codeaurora.org 
> > > Date:   Thu Dec 22 18:41:00 2016 -0800
> > >
> > > scsi: ufs: provide sysfs attribute to select the PM level
> > >
> > > This patch provides the sysfs attribute to choose the power management
> > > level for UFS runtime and system suspend.
> > >
> > > Reviewed-by: Sujit Reddy Thumma 
> > > Signed-off-by: Subhash Jadavani 
> > > Signed-off-by: Martin K. Petersen 
> > >
> > > I just moved it to an another file and changed the sysfs entries
> > > creation by Jaegeuk Kim' request. At the moment the entry shows the PM
> > > level, the device state, the link state and all possible PM levels. Do you
> > want me to change it?
> > 
> > Ah, you are just moving this code around.  Ok, that's fine for this patch, 
> > but
> > please fix it up as part of this patch series because this isn't an 
> > acceptable
> > sysfs file at all.  If it were documented that would be a lot more obvious 
> > as to
> > just how wrong it was :(
> > 
> > And, as it wasn't documented, you can change it as it's obvious no one used 
> > it
> > :)
> > 
> > thanks,
> > 
> > greg k-h
>  
> Can I fix these entries not in this patchset? As long as I know they are used 
> and I
> would prefer that change of the existing sysfs entries behavior be related to 
> a
> separate patch.

Ok, but it's nice to at least hope that someone will fix it up soon :)

thanks,

greg k-h

RE: [PATCH v4 01/10] ufs: sysfs: attribute group for existing sysfs entries.

2018-02-04 Thread Stanislav Nijnikov 


> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Sunday, February 4, 2018 2:34 PM
> To: Stanislav Nijnikov 
> Cc: linux-scsi@vger.kernel.org; linux-ker...@vger.kernel.org;
> jaeg...@kernel.org; Alex Lemberg 
> Subject: Re: [PATCH v4 01/10] ufs: sysfs: attribute group for existing sysfs
> entries.
> 
> On Sun, Feb 04, 2018 at 12:29:06PM +, Stanislav Nijnikov wrote:
> > > > +   curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len),
> > > > +"\nAll available Runtime PM levels 
> > > > info:\n");
> > > > +   for (lvl = UFS_PM_LVL_0; lvl < UFS_PM_LVL_MAX; lvl++)
> > > > +   curr_len += snprintf((buf + curr_len), (PAGE_SIZE - 
> > > > curr_len),
> > > > +"\tRuntime PM level [%d] => 
> > > > dev_state
> > > [%s] link_state [%s]\n",
> > > > +   lvl,
> > > > +   ufschd_ufs_dev_pwr_mode_to_string(
> > > > +   
> > > > ufs_pm_lvl_states[lvl].dev_state),
> > > > +   ufschd_uic_link_state_to_string(
> > > > +   
> > > > ufs_pm_lvl_states[lvl].link_state));
> > > > +
> > >
> > > sysfs if "one value per file", not "random text that someone has to
> > > parse per file" please.
> > >
> > > Huge hint, if you ever care about checking the size of the sysfs
> > > buffer you are writing into, you are doing something really really wrong.
> > >
> > Hi Greg
> > It's the existing code, added by:
> > commit 09690d5a6ae1b7e4cb5ac429c311b99d09352c12
> > Author: subha...@codeaurora.org 
> > Date:   Thu Dec 22 18:41:00 2016 -0800
> >
> > scsi: ufs: provide sysfs attribute to select the PM level
> >
> > This patch provides the sysfs attribute to choose the power management
> > level for UFS runtime and system suspend.
> >
> > Reviewed-by: Sujit Reddy Thumma 
> > Signed-off-by: Subhash Jadavani 
> > Signed-off-by: Martin K. Petersen 
> >
> > I just moved it to an another file and changed the sysfs entries
> > creation by Jaegeuk Kim' request. At the moment the entry shows the PM
> > level, the device state, the link state and all possible PM levels. Do you
> want me to change it?
> 
> Ah, you are just moving this code around.  Ok, that's fine for this patch, but
> please fix it up as part of this patch series because this isn't an acceptable
> sysfs file at all.  If it were documented that would be a lot more obvious as 
> to
> just how wrong it was :(
> 
> And, as it wasn't documented, you can change it as it's obvious no one used it
> :)
> 
> thanks,
> 
> greg k-h
 
Can I fix these entries not in this patchset? As long as I know they are used 
and I
would prefer that change of the existing sysfs entries behavior be related to a
separate patch.

Regards.
Stanislav.

Re: [PATCH v4 01/10] ufs: sysfs: attribute group for existing sysfs entries.

2018-02-04 Thread Greg KH 
On Sun, Feb 04, 2018 at 12:29:06PM +, Stanislav Nijnikov wrote:
> > > + curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len),
> > > +  "\nAll available Runtime PM levels info:\n");
> > > + for (lvl = UFS_PM_LVL_0; lvl < UFS_PM_LVL_MAX; lvl++)
> > > + curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len),
> > > +  "\tRuntime PM level [%d] => dev_state
> > [%s] link_state [%s]\n",
> > > + lvl,
> > > + ufschd_ufs_dev_pwr_mode_to_string(
> > > + ufs_pm_lvl_states[lvl].dev_state),
> > > + ufschd_uic_link_state_to_string(
> > > + ufs_pm_lvl_states[lvl].link_state));
> > > +
> > 
> > sysfs if "one value per file", not "random text that someone has to parse 
> > per
> > file" please.
> > 
> > Huge hint, if you ever care about checking the size of the sysfs buffer you 
> > are
> > writing into, you are doing something really really wrong.
> > 
> Hi Greg
> It's the existing code, added by:
> commit 09690d5a6ae1b7e4cb5ac429c311b99d09352c12
> Author: subha...@codeaurora.org 
> Date:   Thu Dec 22 18:41:00 2016 -0800
> 
> scsi: ufs: provide sysfs attribute to select the PM level
> 
> This patch provides the sysfs attribute to choose the power management
> level for UFS runtime and system suspend.
> 
> Reviewed-by: Sujit Reddy Thumma 
> Signed-off-by: Subhash Jadavani 
> Signed-off-by: Martin K. Petersen 
> 
> I just moved it to an another file and changed the sysfs entries creation by
> Jaegeuk Kim' request. At the moment the entry shows the PM level, the device
> state, the link state and all possible PM levels. Do you want me to change it?

Ah, you are just moving this code around.  Ok, that's fine for this
patch, but please fix it up as part of this patch series because this
isn't an acceptable sysfs file at all.  If it were documented that would
be a lot more obvious as to just how wrong it was :(

And, as it wasn't documented, you can change it as it's obvious no one
used it :)

thanks,

greg k-h

RE: [PATCH v4 01/10] ufs: sysfs: attribute group for existing sysfs entries.

2018-02-04 Thread Stanislav Nijnikov 


> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Thursday, February 1, 2018 6:59 PM
> To: Stanislav Nijnikov 
> Cc: linux-scsi@vger.kernel.org; linux-ker...@vger.kernel.org;
> jaeg...@kernel.org; Alex Lemberg 
> Subject: Re: [PATCH v4 01/10] ufs: sysfs: attribute group for existing sysfs
> entries.
> 
> On Thu, Feb 01, 2018 at 06:15:37PM +0200, Stanislav Nijnikov wrote:
> > This patch introduces attribute group to show existing sysfs entries.
> >
> > Signed-off-by: Stanislav Nijnikov 
> > ---
> >  drivers/scsi/ufs/Makefile|   3 +-
> >  drivers/scsi/ufs/ufs-sysfs.c | 164
> > +++
> >  drivers/scsi/ufs/ufs-sysfs.h |  22 ++
> >  drivers/scsi/ufs/ufshcd.c| 156 ++--
> >  drivers/scsi/ufs/ufshcd.h|   2 +
> >  5 files changed, 194 insertions(+), 153 deletions(-)  create mode
> > 100644 drivers/scsi/ufs/ufs-sysfs.c  create mode 100644
> > drivers/scsi/ufs/ufs-sysfs.h
> >
> > diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
> > index 9310c6c..918f579 100644
> > --- a/drivers/scsi/ufs/Makefile
> > +++ b/drivers/scsi/ufs/Makefile
> > @@ -3,6 +3,7 @@
> >  obj-$(CONFIG_SCSI_UFS_DWC_TC_PCI) += tc-dwc-g210-pci.o ufshcd-
> dwc.o
> > tc-dwc-g210.o
> >  obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o
> > ufshcd-dwc.o tc-dwc-g210.o
> >  obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o
> > -obj-$(CONFIG_SCSI_UFSHCD) += ufshcd.o
> > +obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o ufshcd-core-objs :=
> > +ufshcd.o ufs-sysfs.o
> >  obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
> >  obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o diff --git
> > a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c new file
> > mode 100644 index 000..cc68a90
> > --- /dev/null
> > +++ b/drivers/scsi/ufs/ufs-sysfs.c
> > @@ -0,0 +1,164 @@
> > +//SPDX-License-Identifier: GPL-2.0-only //Copyright (C) 2018 Western
> > +Digital Corporation //This program is free software; you can
> > +redistribute it and/or modify it //under the terms of the GNU General
> > +Public License as published by the //Free Software Foundation;
> > +version 2.
> > +//
> > +//This program is distributed in the hope that it will be useful, but
> > +//WITHOUT ANY WARRANTY; without even the implied warranty of
> > +//MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> GNU
> > +//General Public License for more details.
> 
> No need to put the whole "This program" crud in the file here if you have the
> SPDX line already.  We are going through the kernel tree and removing all of
> the 700+ different ways we have this boilerplate code in the tree, please do
> not add new ones.
> 
> Also, please put a ' ' after "//", it just looks ugly like this, don't you 
> think so?
> 
> Please fix this for all of the files you add in this patch series.
> 
> > +#include 
> > +#include 
> > +
> > +#include "ufs-sysfs.h"
> > +
> > +static const char *ufschd_uic_link_state_to_string(
> > +   enum uic_link_state state)
> > +{
> > +   switch (state) {
> > +   case UIC_LINK_OFF_STATE:return "OFF";
> > +   case UIC_LINK_ACTIVE_STATE: return "ACTIVE";
> > +   case UIC_LINK_HIBERN8_STATE:return "HIBERN8";
> > +   default:return "UNKNOWN";
> > +   }
> > +}
> > +
> > +static const char *ufschd_ufs_dev_pwr_mode_to_string(
> > +   enum ufs_dev_pwr_mode state)
> > +{
> > +   switch (state) {
> > +   case UFS_ACTIVE_PWR_MODE:   return "ACTIVE";
> > +   case UFS_SLEEP_PWR_MODE:return "SLEEP";
> > +   case UFS_POWERDOWN_PWR_MODE:return "POWERDOWN";
> > +   default:return "UNKNOWN";
> > +   }
> > +}
> > +
> > +static inline ssize_t ufs_sysfs_pm_lvl_store(struct device *dev,
> > +struct device_attribute *attr,
> > +const char *buf, size_t count,
> > +bool rpm)
> > +{
> > +   struct ufs_hba *hba = dev_get_drvdata(dev);
> > +   unsigned long flags, value;
> > +
> > +   if (kstrtoul(buf, 0, ))
> > +   return -EINVAL;
> > +
> > +   if (value >= UFS_PM_LVL_MAX)
> > +   return -EINVAL;
> > +
> > +   spin_lock_irqsave(hba->host->host_lock, flags);
> > +   if (rpm)
> > +   hba->rpm_lvl = value;
> > +   else
> > +   hba->spm_lvl = value;
> > +   spin_unlock_irqrestore(hba->host->host_lock, flags);
> > +   return count;
> > +}
> > +
> > +static ssize_t rpm_lvl_show(struct device *dev,
> > +   struct device_attribute *attr, char *buf) {
> > +   struct ufs_hba *hba = dev_get_drvdata(dev);
> > +   int curr_len;
> > +   u8 lvl;
> > +
> > +   curr_len = snprintf(buf, PAGE_SIZE,
> > +   "\nCurrent Runtime PM level [%d] => dev_state
> [%s] link_state [%s]\n",
> > +

Re: [PATCH v4 02/10] ufs: sysfs: device descriptor

2018-02-04 Thread gre...@linuxfoundation.org 
On Sun, Feb 04, 2018 at 09:03:25AM +, Stanislav Nijnikov wrote:
> > -Original Message-
> > From: Bart Van Assche
> > Sent: Friday, February 2, 2018 6:32 PM
> > To: gre...@linuxfoundation.org
> > Cc: linux-scsi@vger.kernel.org; linux-ker...@vger.kernel.org;
> > jaeg...@kernel.org; Alex Lemberg ; Stanislav
> > Nijnikov 
> > Subject: Re: [PATCH v4 02/10] ufs: sysfs: device descriptor
> > 
> > On Fri, 2018-02-02 at 08:17 +0100, gre...@linuxfoundation.org wrote:
> > > On Fri, Feb 02, 2018 at 12:25:46AM +, Bart Van Assche wrote:
> > > > On Thu, 2018-02-01 at 18:15 +0200, Stanislav Nijnikov wrote:
> > > > > +enum ufs_desc_param_size {
> > > > > + UFS_PARAM_BYTE_SIZE = 1,
> > > > > + UFS_PARAM_WORD_SIZE = 2,
> > > > > + UFS_PARAM_DWORD_SIZE= 4,
> > > > > + UFS_PARAM_QWORD_SIZE= 8,
> > > > > +};
> > > >
> > > > Please do not copy bad naming choices from the Windows kernel into
> > > > the Linux kernel. Using names like WORD / DWORD / QWORD is much less
> > > > readable than using the numeric constants 2, 4, 8. Hence my proposal
> > > > to leave out the above enum completely.
> > >
> > > Are you sure those do not come from the spec itself?  It's been a
> > > while since I last read it, but for some reason I remember those types
> > > of names being in there.  But I might be confusing specs here.
> > 
> > Hello Greg,
> > 
> > That's a good question. However, a quick search on the Internet for the
> > search phrase "Universal Flash Storage" "qword" did not yield any results
> > about UFS in the first ten search hits. And I haven't found any references 
> > to
> > the DWORD / QWORD terminology in the "UNIVERSAL FLASH STORAGE HOST
> > CONTROLLER INTERFACE (UFSHCI), UNIFIED MEMORY EXTENSION, Version
> > 1.1" document either. Maybe that means that I was looking at the wrong
> > document?
> > 
> > Thanks,
> > 
> > Bart.
> > 
> > 
> The UFS spec 2.1 specifies size as first letter in names of the descriptor 
> parameters and attributes (e.g. bDeviceClass, wSpecVersion, dPSAMaxDataSize, 
> qTotalRawDeviceCapacity, ...). But usage of the enum could be easily removed.

It matches the naming scheme of the spec, so in my opinion, it's fine
as-is.  But as I'm not the author here, it's up to you what you want to
use, you have to maintain this, not me :)

thanks,

greg k-h

Re: scsi: sg: assorted memory corruptions

2018-02-04 Thread Dmitry Vyukov 
On Sun, Feb 4, 2018 at 10:07 AM, Eric Biggers  wrote:
> On Thu, Feb 01, 2018 at 05:21:12PM +0100, 'Dmitry Vyukov' via syzkaller wrote:
>> On Thu, Feb 1, 2018 at 5:17 PM, Ben Hutchings
>>  wrote:
>> > On Thu, 2018-02-01 at 08:04 +0100, Dmitry Vyukov wrote:
>> >> On Thu, Feb 1, 2018 at 7:03 AM, Douglas Gilbert  
>> >> wrote:
>> >> > On 2018-01-30 07:22 AM, Dmitry Vyukov wrote:
>> > [...]
>> >> > > [1:0:0:0]cd/dvd  QEMU QEMU DVD-ROM 2.0.  /dev/sr0   
>> >> > > /dev/sg1
>> >> > >
>> >> > > # readlink /sys/class/scsi_generic/sg0
>> >> > >
>> >> > > ../../devices/pci:00/:00:01.1/ata1/host0/target0:0:0/0:0:0:0/scsi_generic/sg0
>> >> > >
>> >> > > # cat /sys/class/scsi_generic/sg0/device/vendor
>> >> > > ATA
>> >> >
>> >> >
>> >> > ^
>> >> > That subsystem is the culprit IMO, most likely libata.
>> >> >
>> >> > Until you can show this test failing on something other than an
>> >> > ATA disk, then I will treat this issue as closed.
>> >>
>> >> Hi Doug,
>> >>
>> >> Why is bug in ATA not a bug? Is it long unused by everybody? I've got
>> >> it by running qemu with default flags...
>> >
>> > If the bug is in libata then it's not on Doug to fix it since he's only
>> > maintaining sg.
>>
>>
>> Then I think we need to CC ata maintainers rather than treat it as closed.
>> +Tejun, linux-ide@, you can see full thread here:
>> https://groups.google.com/forum/#!topic/syzkaller/9RNr9Gu0MyY
>>
>
> To get memory corruption it's actually sufficient just to submit "1-byte" 
> reads;
> there's no need for the SG_NEXT_CMD_LEN ioctl or anything:
>
> #include 
> #include 
>
> int main()
> {
> int fd = open("/dev/sg0", O_RDWR);
> char buf[43] = { [36] = 0x08 /* READ_6 */ };
>
> for (;;)
> write(fd, buf, sizeof(buf));
> }
>
> (where /dev/sg0 is the default QEMU disk type, "82371SB PIIX3 IDE")
>
> The SCSI command descriptor block is the 6 bytes at indices 36-41, so index 42
> is the only data byte.
>
> Also this is a different bug from the crash in ata_bmdma_fill_sg() which is
> fixed by "libata: fix length validation of ATAPI-relayed SCSI commands".
>
> I'm guessing the driver is DMA'ing to somewhere it shouldn't be...

It would be good to add KASAN checks to the DMA code that issues
transfers. This is another case where a silent memory corruption
causes dozens of assorted crashes all over the kernel. If we add
checks, KASAN would pinpoint the exact stack that issues the bad
command. This may be the simplest way to debug this bug as well. I've
filed https://bugzilla.kernel.org/show_bug.cgi?id=198661 for this.

Re: scsi: sg: assorted memory corruptions

2018-02-04 Thread Eric Biggers 
On Thu, Feb 01, 2018 at 05:21:12PM +0100, 'Dmitry Vyukov' via syzkaller wrote:
> On Thu, Feb 1, 2018 at 5:17 PM, Ben Hutchings
>  wrote:
> > On Thu, 2018-02-01 at 08:04 +0100, Dmitry Vyukov wrote:
> >> On Thu, Feb 1, 2018 at 7:03 AM, Douglas Gilbert  
> >> wrote:
> >> > On 2018-01-30 07:22 AM, Dmitry Vyukov wrote:
> > [...]
> >> > > [1:0:0:0]cd/dvd  QEMU QEMU DVD-ROM 2.0.  /dev/sr0   
> >> > > /dev/sg1
> >> > >
> >> > > # readlink /sys/class/scsi_generic/sg0
> >> > >
> >> > > ../../devices/pci:00/:00:01.1/ata1/host0/target0:0:0/0:0:0:0/scsi_generic/sg0
> >> > >
> >> > > # cat /sys/class/scsi_generic/sg0/device/vendor
> >> > > ATA
> >> >
> >> >
> >> > ^
> >> > That subsystem is the culprit IMO, most likely libata.
> >> >
> >> > Until you can show this test failing on something other than an
> >> > ATA disk, then I will treat this issue as closed.
> >>
> >> Hi Doug,
> >>
> >> Why is bug in ATA not a bug? Is it long unused by everybody? I've got
> >> it by running qemu with default flags...
> >
> > If the bug is in libata then it's not on Doug to fix it since he's only
> > maintaining sg.
> 
> 
> Then I think we need to CC ata maintainers rather than treat it as closed.
> +Tejun, linux-ide@, you can see full thread here:
> https://groups.google.com/forum/#!topic/syzkaller/9RNr9Gu0MyY
> 

To get memory corruption it's actually sufficient just to submit "1-byte" reads;
there's no need for the SG_NEXT_CMD_LEN ioctl or anything:

#include 
#include 

int main()
{
int fd = open("/dev/sg0", O_RDWR);
char buf[43] = { [36] = 0x08 /* READ_6 */ };

for (;;)
write(fd, buf, sizeof(buf));
}

(where /dev/sg0 is the default QEMU disk type, "82371SB PIIX3 IDE")

The SCSI command descriptor block is the 6 bytes at indices 36-41, so index 42
is the only data byte.

Also this is a different bug from the crash in ata_bmdma_fill_sg() which is
fixed by "libata: fix length validation of ATAPI-relayed SCSI commands".

I'm guessing the driver is DMA'ing to somewhere it shouldn't be...

Eric

RE: [PATCH v4 02/10] ufs: sysfs: device descriptor

2018-02-04 Thread Stanislav Nijnikov 
> -Original Message-
> From: Bart Van Assche
> Sent: Friday, February 2, 2018 6:32 PM
> To: gre...@linuxfoundation.org
> Cc: linux-scsi@vger.kernel.org; linux-ker...@vger.kernel.org;
> jaeg...@kernel.org; Alex Lemberg ; Stanislav
> Nijnikov 
> Subject: Re: [PATCH v4 02/10] ufs: sysfs: device descriptor
> 
> On Fri, 2018-02-02 at 08:17 +0100, gre...@linuxfoundation.org wrote:
> > On Fri, Feb 02, 2018 at 12:25:46AM +, Bart Van Assche wrote:
> > > On Thu, 2018-02-01 at 18:15 +0200, Stanislav Nijnikov wrote:
> > > > +enum ufs_desc_param_size {
> > > > +   UFS_PARAM_BYTE_SIZE = 1,
> > > > +   UFS_PARAM_WORD_SIZE = 2,
> > > > +   UFS_PARAM_DWORD_SIZE= 4,
> > > > +   UFS_PARAM_QWORD_SIZE= 8,
> > > > +};
> > >
> > > Please do not copy bad naming choices from the Windows kernel into
> > > the Linux kernel. Using names like WORD / DWORD / QWORD is much less
> > > readable than using the numeric constants 2, 4, 8. Hence my proposal
> > > to leave out the above enum completely.
> >
> > Are you sure those do not come from the spec itself?  It's been a
> > while since I last read it, but for some reason I remember those types
> > of names being in there.  But I might be confusing specs here.
> 
> Hello Greg,
> 
> That's a good question. However, a quick search on the Internet for the
> search phrase "Universal Flash Storage" "qword" did not yield any results
> about UFS in the first ten search hits. And I haven't found any references to
> the DWORD / QWORD terminology in the "UNIVERSAL FLASH STORAGE HOST
> CONTROLLER INTERFACE (UFSHCI), UNIFIED MEMORY EXTENSION, Version
> 1.1" document either. Maybe that means that I was looking at the wrong
> document?
> 
> Thanks,
> 
> Bart.
> 
> 
The UFS spec 2.1 specifies size as first letter in names of the descriptor 
parameters and attributes (e.g. bDeviceClass, wSpecVersion, dPSAMaxDataSize, 
qTotalRawDeviceCapacity, ...). But usage of the enum could be easily removed.

Regards
Stanislav