Re: [PATCH 6/9] block: Add 'zoned' sysfs queue attribute

[Hannes Reinecke]

On 04/07/2016 03:56 AM, Damien Le Moal wrote:
> 
> Hannes,
> 
>> Add a sysfs queue attribute 'zoned' to display the zone layout
>> for zoned devices.
>>
>> Signed-off-by: Hannes Reinecke 
>> ---
>> block/blk-sysfs.c | 47 +++
>> 1 file changed, 47 insertions(+)
>>
>> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
>> index ff97091..748bb27 100644
>> --- a/block/blk-sysfs.c
>> +++ b/block/blk-sysfs.c
>> @@ -244,6 +244,43 @@ static ssize_t queue_max_hw_sectors_show(struct 
>> request_queue *q, char *page)
>>  return queue_var_show(max_hw_sectors_kb, (page));
>> }
>>
>> +#ifdef CONFIG_BLK_DEV_ZONED
>> +static ssize_t queue_zoned_show(struct request_queue *q, char *page)
>> +{
>> +struct rb_node *node;
>> +struct blk_zone *zone;
>> +ssize_t offset = 0, end = 0;
>> +size_t size = 0, num = 0;
>> +enum blk_zone_type type = BLK_ZONE_TYPE_UNKNOWN;
>> +
>> +for (node = rb_first(>zones); node; node = rb_next(node)) {
>> +zone = rb_entry(node, struct blk_zone, node);
>> +if (zone->type != type ||
>> +zone->len != size ||
>> +end != zone->start) {
>> +if (size != 0)
>> +offset += sprintf(page + offset, "%zu\n", num);
>> +/* We can only store one page ... */
>> +if (offset + 42 > PAGE_SIZE) {
>> +offset += sprintf(page + offset, "...\n");
>> +return offset;
>> +}
>> +size = zone->len;
>> +type = zone->type;
>> +offset += sprintf(page + offset, "%zu %zu %d ",
>> +  zone->start, size, type);
>> +num = 0;
>> +end = zone->start + size;
>> +} else
>> +end += zone->len;
>> +num++;
>> +}
>> +if (num > 0)
>> +offset += sprintf(page + offset, "%zu\n", num);
>> +return offset > 0 ? offset : -EINVAL;
>> +}
>> +#endif
> 
> With this, an application reading the “zoned” file for a non-SMR disk
> will get a -EINVAL error. Not really super nice. Could we just have the
> zoned files contain a single “0” for non-SMR disks ? Or at least have the
> file empty and read return 0 Bytes. That would allow applications to easily
> and cleanly detect if they are dealing with a SMR disk (or not) instead of
> assuming that “-EINVAL” means “not SMR”, which seems very ugly to me.
> 
Sure.

Actually I was looking in blanking out this attribute entirely like
I did with the SCSI sysfs attributes, but as the 'queue' sysfs
directory is a blank kobj it's not easily done.
But yes, '0' seems like a reasonable value here.

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 10/12] Limit bio_endio recursion

[Ming Lei]

On Thu, 7 Apr 2016 11:54:49 +0800
Ming Lei  wrote:

> On Mon, Apr 4, 2016 at 1:06 PM, Shaun Tancheff  wrote:
> > Recursive endio calls can exceed 16k stack. Tested with
> > 32k stack and observed:
> >
> > DepthSize   Location(293 entries)
> > -   
> >   0)21520  16   __module_text_address+0x12/0x60
> >   1)21504   8   __module_address+0x5/0x140
> >   2)21496  24   __module_text_address+0x12/0x60
> >   3)21472  16   is_module_text_address+0xe/0x20
> >   4)21456   8   __kernel_text_address+0x50/0x80
> >   5)21448 136   print_context_stack+0x5a/0xf0
> >   6)21312 144   dump_trace+0x14c/0x300
> >   7)21168   8   save_stack_trace+0x2f/0x50
> >   8)21160  88   set_track+0x64/0x130
> >   9)21072  96   free_debug_processing+0x200/0x290
> >  10)20976 176   __slab_free+0x164/0x290
> >  11)20800  48   kmem_cache_free+0x1b0/0x1e0
> >  12)20752  16   mempool_free_slab+0x17/0x20
> >  13)20736  48   mempool_free+0x2f/0x90
> >  14)20688  16   bvec_free+0x36/0x40
> >  15)20672  32   bio_free+0x3b/0x60
> >  16)20640  16   bio_put+0x23/0x30
> >  17)20624  64   end_bio_extent_writepage+0xcf/0xe0
> >  18)20560  48   bio_endio+0x57/0x90
> >  19)20512  48   btrfs_end_bio+0xa8/0x160
> >  20)20464  48   bio_endio+0x57/0x90
> >  21)20416 112   dec_pending+0x121/0x270
> >  22)20304  64   clone_endio+0x7a/0x100
> >  23)20240  48   bio_endio+0x57/0x90
> > ...
> > 277) 1264  64   clone_endio+0x7a/0x100
> > 278) 1200  48   bio_endio+0x57/0x90
> > 279) 1152 112   dec_pending+0x121/0x270
> > 280) 1040  64   clone_endio+0x7a/0x100
> > 281)  976  48   bio_endio+0x57/0x90
> > 282)  928  80   blk_update_request+0x8f/0x340
> > 283)  848  80   scsi_end_request+0x33/0x1c0
> > 284)  768 112   scsi_io_completion+0xb5/0x620
> > 285)  656  48   scsi_finish_command+0xcf/0x120
> > 286)  608  48   scsi_softirq_done+0x126/0x150
> > 287)  560  24   blk_done_softirq+0x78/0x90
> > 288)  536 136   __do_softirq+0xfd/0x280
> > 289)  400  16   run_ksoftirqd+0x28/0x50
> > 290)  384  64   smpboot_thread_fn+0x105/0x160
> > 291)  320 144   kthread+0xc9/0xe0
> > 292)  176 176   ret_from_fork+0x3f/0x70
> >
> > Based on earlier patch by Mikulas Patocka .
> > https://lkml.org/lkml/2008/6/24/18
> 
> Looks a empty link, and the following had the discussion too:
> 
> http://linux-kernel.2935.n7.nabble.com/PATCH-1-2-Avoid-bio-endio-recursion-td306043.html
> 
> >
> > Signed-off-by: Shaun Tancheff 
> > ---
> >  block/bio.c | 57 ++---
> >  1 file changed, 54 insertions(+), 3 deletions(-)
> >
> > diff --git a/block/bio.c b/block/bio.c
> > index d42e69c..4ac19f6 100644
> > --- a/block/bio.c
> > +++ b/block/bio.c
> > @@ -1733,6 +1733,59 @@ static inline bool bio_remaining_done(struct bio 
> > *bio)
> > return false;
> >  }
> >
> > +static DEFINE_PER_CPU(struct bio **, bio_end_queue) = { NULL };
> 
> The idea looks very nice, but this patch can't be applid to current block-next
> branch.
> 
> Looks it might be simpler to implement the approach by using percpu bio_list.

Cc Mikulas & Christoph

How about the following implementation with bio_list? Looks more readable and
simpler.

Just run a recent testcase of bcache over raid1(virtio-blk, virtio-blk), looks
it does work, :-)

---

diff --git a/block/bio.c b/block/bio.c
index f124a0a..b97dfe8 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -68,6 +68,8 @@ static DEFINE_MUTEX(bio_slab_lock);
 static struct bio_slab *bio_slabs;
 static unsigned int bio_slab_nr, bio_slab_max;
 
+static DEFINE_PER_CPU(struct bio_list *, bio_end_list) = { NULL };
+
 static struct kmem_cache *bio_find_or_create_slab(unsigned int extra_size)
 {
unsigned int sz = sizeof(struct bio) + extra_size;
@@ -1737,6 +1739,46 @@ static inline bool bio_remaining_done(struct bio *bio)
return false;
 }
 
+/* disable local irq when manipulating the percpu bio_list */
+static void unwind_bio_endio(struct bio *bio)
+{
+   struct bio_list bl_in_stack;
+   struct bio_list *bl;
+   unsigned long flags;
+   bool clear_list = false;
+
+   local_irq_save(flags);
+
+   bl = this_cpu_read(bio_end_list);
+   if (!bl) {
+   bl = _in_stack;
+   bio_list_init(bl);
+   clear_list = true;
+   }
+
+   if (!bio_list_empty(bl)) {
+   bio_list_add(bl, bio);
+   goto out;
+   }
+
+   while (1) {
+   

Re: [PATCH 09/27] target: use bio_is_full()

[Ming Lei]

On Tue, Apr 5, 2016 at 9:02 PM, Christoph Hellwig  wrote:
> On Tue, Apr 05, 2016 at 07:56:54PM +0800, Ming Lei wrote:
>> +++ b/drivers/target/target_core_pscsi.c
>> @@ -951,7 +951,7 @@ pscsi_map_sg(struct se_cmd *cmd, struct scatterlist 
>> *sgl, u32 sgl_nents,
>>   pr_debug("PSCSI: bio->bi_vcnt: %d nr_vecs: %d\n",
>>   bio->bi_vcnt, nr_vecs);
>>
>> - if (bio->bi_vcnt > nr_vecs) {
>> + if (bio_is_full(bio)) {
>>   pr_debug("PSCSI: Reached bio->bi_vcnt max:"
>>   " %d i: %d bio: %p, allocating another"
>>   " bio\n", bio->bi_vcnt, i, bio);
>
> This check should be removed entirely - bio_add_pc_page takes care of
> it.

OK.


-- 
Ming Lei
--
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 10/12] Limit bio_endio recursion

[Ming Lei]

On Mon, Apr 4, 2016 at 1:06 PM, Shaun Tancheff  wrote:
> Recursive endio calls can exceed 16k stack. Tested with
> 32k stack and observed:
>
> DepthSize   Location(293 entries)
> -   
>   0)21520  16   __module_text_address+0x12/0x60
>   1)21504   8   __module_address+0x5/0x140
>   2)21496  24   __module_text_address+0x12/0x60
>   3)21472  16   is_module_text_address+0xe/0x20
>   4)21456   8   __kernel_text_address+0x50/0x80
>   5)21448 136   print_context_stack+0x5a/0xf0
>   6)21312 144   dump_trace+0x14c/0x300
>   7)21168   8   save_stack_trace+0x2f/0x50
>   8)21160  88   set_track+0x64/0x130
>   9)21072  96   free_debug_processing+0x200/0x290
>  10)20976 176   __slab_free+0x164/0x290
>  11)20800  48   kmem_cache_free+0x1b0/0x1e0
>  12)20752  16   mempool_free_slab+0x17/0x20
>  13)20736  48   mempool_free+0x2f/0x90
>  14)20688  16   bvec_free+0x36/0x40
>  15)20672  32   bio_free+0x3b/0x60
>  16)20640  16   bio_put+0x23/0x30
>  17)20624  64   end_bio_extent_writepage+0xcf/0xe0
>  18)20560  48   bio_endio+0x57/0x90
>  19)20512  48   btrfs_end_bio+0xa8/0x160
>  20)20464  48   bio_endio+0x57/0x90
>  21)20416 112   dec_pending+0x121/0x270
>  22)20304  64   clone_endio+0x7a/0x100
>  23)20240  48   bio_endio+0x57/0x90
> ...
> 277) 1264  64   clone_endio+0x7a/0x100
> 278) 1200  48   bio_endio+0x57/0x90
> 279) 1152 112   dec_pending+0x121/0x270
> 280) 1040  64   clone_endio+0x7a/0x100
> 281)  976  48   bio_endio+0x57/0x90
> 282)  928  80   blk_update_request+0x8f/0x340
> 283)  848  80   scsi_end_request+0x33/0x1c0
> 284)  768 112   scsi_io_completion+0xb5/0x620
> 285)  656  48   scsi_finish_command+0xcf/0x120
> 286)  608  48   scsi_softirq_done+0x126/0x150
> 287)  560  24   blk_done_softirq+0x78/0x90
> 288)  536 136   __do_softirq+0xfd/0x280
> 289)  400  16   run_ksoftirqd+0x28/0x50
> 290)  384  64   smpboot_thread_fn+0x105/0x160
> 291)  320 144   kthread+0xc9/0xe0
> 292)  176 176   ret_from_fork+0x3f/0x70
>
> Based on earlier patch by Mikulas Patocka .
> https://lkml.org/lkml/2008/6/24/18

Looks a empty link, and the following had the discussion too:

http://linux-kernel.2935.n7.nabble.com/PATCH-1-2-Avoid-bio-endio-recursion-td306043.html

>
> Signed-off-by: Shaun Tancheff 
> ---
>  block/bio.c | 57 ++---
>  1 file changed, 54 insertions(+), 3 deletions(-)
>
> diff --git a/block/bio.c b/block/bio.c
> index d42e69c..4ac19f6 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -1733,6 +1733,59 @@ static inline bool bio_remaining_done(struct bio *bio)
> return false;
>  }
>
> +static DEFINE_PER_CPU(struct bio **, bio_end_queue) = { NULL };

The idea looks very nice, but this patch can't be applid to current block-next
branch.

Looks it might be simpler to implement the approach by using percpu bio_list.

> +
> +static struct bio *unwind_bio_endio(struct bio *bio)
> +{
> +   struct bio ***bio_end_queue_ptr;
> +   struct bio *bio_queue;
> +   struct bio *chain_bio = NULL;
> +   int error = bio->bi_error;
> +   unsigned long flags;
> +
> +   local_irq_save(flags);

If we may resue current->bio_list for this purpose, disabling irq should have
been avoided, but looks it is difficult to do in that way, maybe impossible.
Also not realistic to introduce a new per-task variable.

> +   bio_end_queue_ptr = this_cpu_ptr(_end_queue);
> +
> +   if (*bio_end_queue_ptr) {
> +   **bio_end_queue_ptr = bio;
> +   *bio_end_queue_ptr = >bi_next;
> +   bio->bi_next = NULL;
> +   } else {
> +   bio_queue = NULL;
> +   *bio_end_queue_ptr = _queue;

Suggest to comment that bio_queue is the bottom-most stack variable,
otherwise looks a bit tricky to understand.

> +
> +next_bio:
> +   if (bio->bi_end_io == bio_chain_endio) {
> +   struct bio *parent = bio->bi_private;
> +
> +   bio_put(bio);
> +   chain_bio = parent;
> +   goto out;
> +   }
> +
> +   if (bio->bi_end_io) {
> +   if (!bio->bi_error)
> +   bio->bi_error = error;
> +   bio->bi_end_io(bio);
> +   }
> +
> +   if (bio_queue) {
> +   bio = bio_queue;
> +   bio_queue = bio->bi_next;
> +   

Re: [PATCH 6/9] block: Add 'zoned' sysfs queue attribute

[Damien Le Moal]

Hannes,

>Add a sysfs queue attribute 'zoned' to display the zone layout
>for zoned devices.
>
>Signed-off-by: Hannes Reinecke 
>---
> block/blk-sysfs.c | 47 +++
> 1 file changed, 47 insertions(+)
>
>diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
>index ff97091..748bb27 100644
>--- a/block/blk-sysfs.c
>+++ b/block/blk-sysfs.c
>@@ -244,6 +244,43 @@ static ssize_t queue_max_hw_sectors_show(struct 
>request_queue *q, char *page)
>   return queue_var_show(max_hw_sectors_kb, (page));
> }
> 
>+#ifdef CONFIG_BLK_DEV_ZONED
>+static ssize_t queue_zoned_show(struct request_queue *q, char *page)
>+{
>+  struct rb_node *node;
>+  struct blk_zone *zone;
>+  ssize_t offset = 0, end = 0;
>+  size_t size = 0, num = 0;
>+  enum blk_zone_type type = BLK_ZONE_TYPE_UNKNOWN;
>+
>+  for (node = rb_first(>zones); node; node = rb_next(node)) {
>+  zone = rb_entry(node, struct blk_zone, node);
>+  if (zone->type != type ||
>+  zone->len != size ||
>+  end != zone->start) {
>+  if (size != 0)
>+  offset += sprintf(page + offset, "%zu\n", num);
>+  /* We can only store one page ... */
>+  if (offset + 42 > PAGE_SIZE) {
>+  offset += sprintf(page + offset, "...\n");
>+  return offset;
>+  }
>+  size = zone->len;
>+  type = zone->type;
>+  offset += sprintf(page + offset, "%zu %zu %d ",
>+zone->start, size, type);
>+  num = 0;
>+  end = zone->start + size;
>+  } else
>+  end += zone->len;
>+  num++;
>+  }
>+  if (num > 0)
>+  offset += sprintf(page + offset, "%zu\n", num);
>+  return offset > 0 ? offset : -EINVAL;
>+}
>+#endif

With this, an application reading the “zoned” file for a non-SMR disk
will get a -EINVAL error. Not really super nice. Could we just have the
zoned files contain a single “0” for non-SMR disks ? Or at least have the
file empty and read return 0 Bytes. That would allow applications to easily
and cleanly detect if they are dealing with a SMR disk (or not) instead of
assuming that “-EINVAL” means “not SMR”, which seems very ugly to me.

Best.


--
Damien Le Moal, Ph.D.
Sr Manager, System Software Group, WW Research,
HGST, a Western Digital company
damien.lem...@hgst.com
Tel: (+81) 0466-98-3593 (Ext. 51-3593)
1 kirihara-cho, Fujisawa, Kanagawa, 252-0888 Japan
www.hgst.com 
Western Digital Corporation (and its subsidiaries) E-mail Confidentiality 
Notice & Disclaimer:

This e-mail and any files transmitted with it may contain confidential or 
legally privileged information of WDC and/or its affiliates, and are intended 
solely for the use of the individual or entity to which they are addressed. If 
you are not the intended recipient, any disclosure, copying, distribution or 
any action taken or omitted to be taken in reliance on it, is prohibited. If 
you have received this e-mail in error, please notify the sender immediately 
and delete the e-mail in its entirety from your system.

Re: [PATCH] csiostor: Fix backwards locking in the function __csio_unreg_rnode

[Theodore Ts'o]

On Wed, Apr 06, 2016 at 11:41:49AM -0700, Greg KH wrote:
> > > > Is this Nick Krause?  An email reply that Martin forwarded but the 
> > > > list didn't pick up (because it had a html part) suggests this. 
> > > >  What you're doing is what got you banned from LKML the last time: 
> > > > sending patches without evidence there's a problem or understanding 
> > > > the code you're patching.  Repeating the behaviour under a new 
> > > > identity isn't going to help improve your standing.
> > > > 
> > > No I am not Nick Krause. I am just aware of how he got banned a few 
> > > years ago. That email was a mistake by typo and was hoping nobody 
> > > picked it up as they would then believe I was Nick Krause.
> > 
> > Hm, OK, but currently you are repeating his behaviour ... please don't
> > send any more patches until they're about real problems backed by
> > actual data.
> 
> He's Nick, look at the email headers for proof.
> 
> James, and everyone else, please drop his patches.  I'll go get him
> banned from vger again.
> 
> Nick, please stop, you have violated the DCO now, a much worse thing
> than before.

Even if Bastien is going to try to claim that he happens to live in
the same house as Nick, the following two patches clearly shows that
even if Bastien is violating the DCO by using a fake sock puppet, he's
violated the DCO by failing to give Nick credit for writing the
identical patch.

- Ted



evidence.gz
Description: application/gzip

[Bug 114401] crash dump aic94xx

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=114401

bastienphilb...@gmail.com changed:

   What|Removed |Added

 CC||bastienphilb...@gmail.com

--- Comment #1 from bastienphilb...@gmail.com ---
out_done:
 tascb->completion = NULL;
 if (res == TMF_RESP_FUNC_COMPLETE) {
 task->lldd_task = NULL;
add these line:
 list_del_init(>list);
 mb();
 asd_ascb_free(tascb);
 }
 ASD_DPRINTK("task 0x%p aborted, res: 0x%x\n", task, res);
 return res;

  out_free:
same here:
 list_del_init(>list);
 asd_ascb_free(ascb);
 ASD_DPRINTK("task 0x%p aborted, res: 0x%x\n", task, res);
 return res;
}
Tell me if this fixes your issue.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
--
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] csiostor: Fix backwards locking in the function __csio_unreg_rnode

[Greg KH]

On Wed, Apr 06, 2016 at 01:28:24PM -0400, James Bottomley wrote:
> On Wed, 2016-04-06 at 13:23 -0400, Bastien Philbert wrote:
> > 
> > On 2016-04-06 01:14 PM, James Bottomley wrote:
> > > On Wed, 2016-04-06 at 10:36 -0400, Bastien Philbert wrote:
> > > > 
> > > > On 2016-04-06 10:24 AM, James Bottomley wrote:
> > > > > On Wed, 2016-04-06 at 10:11 -0400, Bastien Philbert wrote:
> > > > > > 
> > > > > > On 2016-04-06 09:38 AM, James Bottomley wrote:
> > > > > > > On Wed, 2016-04-06 at 09:21 -0400, Bastien Philbert wrote:
> > > > > > > > 
> > > > > > > > On 2016-04-06 03:48 AM, Julian Calaby wrote:
> > > > > > > > > Hi Bastien,
> > > > > > > > > 
> > > > > > > > > On Wed, Apr 6, 2016 at 7:19 AM, Bastien Philbert
> > > > > > > > >  wrote:
> > > > > > > > > > This fixes backwards locking in the function
> > > > > > > > > > __csio_unreg_rnode
> > > > > > > > > > to
> > > > > > > > > > properly lock before the call to the function
> > > > > > > > > > csio_unreg_rnode
> > > > > > > > > > and
> > > > > > > > > > not unlock with spin_unlock_irq as this would not
> > > > > > > > > > allow
> > > > > > > > > > the
> > > > > > > > > > proper
> > > > > > > > > > protection for concurrent access on the shared
> > > > > > > > > > csio_hw
> > > > > > > > > > structure
> > > > > > > > > > pointer hw. In addition switch the locking after the
> > > > > > > > > > critical
> > > > > > > > > > region
> > > > > > > > > > function call to properly unlock instead with
> > > > > > > > > > spin_unlock_irq
> > > > > > > > > > on
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Bastien Philbert <
> > > > > > > > > > bastienphilb...@gmail.com>
> > > > > > > > > > ---
> > > > > > > > > >  drivers/scsi/csiostor/csio_rnode.c | 4 ++--
> > > > > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/drivers/scsi/csiostor/csio_rnode.c
> > > > > > > > > > b/drivers/scsi/csiostor/csio_rnode.c
> > > > > > > > > > index e9c3b04..029a09e 100644
> > > > > > > > > > --- a/drivers/scsi/csiostor/csio_rnode.c
> > > > > > > > > > +++ b/drivers/scsi/csiostor/csio_rnode.c
> > > > > > > > > > @@ -580,9 +580,9 @@ __csio_unreg_rnode(struct
> > > > > > > > > > csio_rnode
> > > > > > > > > > *rn)
> > > > > > > > > > ln->last_scan_ntgts--;
> > > > > > > > > > }
> > > > > > > > > > 
> > > > > > > > > > -   spin_unlock_irq(>lock);
> > > > > > > > > > -   csio_unreg_rnode(rn);
> > > > > > > > > > spin_lock_irq(>lock);
> > > > > > > > > > +   csio_unreg_rnode(rn);
> > > > > > > > > > +   spin_unlock_irq(>lock);
> > > > > > > > > 
> > > > > > > > > Are you _certain_ this is correct? This construct
> > > > > > > > > usually
> > > > > > > > > appears
> > > > > > > > > when
> > > > > > > > > a function has a particular lock held, then needs to
> > > > > > > > > unlock
> > > > > > > > > it
> > > > > > > > > to
> > > > > > > > > call
> > > > > > > > > some other function. Are you _certain_ that this isn't
> > > > > > > > > the
> > > > > > > > > case?
> > > > > > > > > 
> > > > > > > > > Thanks,
> > > > > > > > > 
> > > > > > > > Yes I am pretty certain this is correct. I checked the
> > > > > > > > paths
> > > > > > > > that
> > > > > > > > called this function
> > > > > > > > and it was weired that none of them gradded the spinlock
> > > > > > > > before
> > > > > > > > hand.
> > > > > > > 
> > > > > > > That's not good enough.  If your theory is correct, lockdep
> > > > > > > should
> > > > > > > be
> > > > > > > dropping an already unlocked assertion in this codepath ...
> > > > > > > do
> > > > > > > you
> > > > > > > see
> > > > > > > this?
> > > > > > > 
> > > > > > > James
> > > > > > > 
> > > > > > > 
> > > > > > Yes I do.
> > > > > 
> > > > > You mean you don't see the lockdep assert, since you're asking
> > > > > to 
> > > > > drop the patch?
> > > > > 
> > > > > >  For now just drop the patch but I am still concerned that we
> > > > > > are
> > > > > > double unlocking here.
> > > > > 
> > > > > Really, no.  The pattern in the code indicates the lock is
> > > > > expected 
> > > > > to be held.  This can be wrong (sometimes code moves or people
> > > > > forget), but if it is wrong we'll get an assert about unlock of
> > > > > an 
> > > > > already unlocked lock.  If there's no assert, the lock is held
> > > > > on 
> > > > > entry and the code is correct.
> > > > > 
> > > > > You're proposing patches based on misunderstandings of the code
> > > > > which aren't backed up by actual issues and wasting everyone's
> > > > > time 
> > > > > to look at them.  Please begin with the hard evidence of a
> > > > > problem 
> > > > > first, so post the lockdep assert in the changelog so we know 
> > > > > there's a real problem.
> > > > > 
> > > > > James
> > > > > 
> > > > Certainly James. I think I just got carried away with the last
> > > > few 
> > > > patches :(.
> > > 
> > > Is this Nick Krause?  An email reply that Martin 

Re: [PATCH] csiostor: Fix backwards locking in the function __csio_unreg_rnode

[Bastien Philbert]

On 2016-04-06 01:28 PM, James Bottomley wrote:
> On Wed, 2016-04-06 at 13:23 -0400, Bastien Philbert wrote:
>>
>> On 2016-04-06 01:14 PM, James Bottomley wrote:
>>> On Wed, 2016-04-06 at 10:36 -0400, Bastien Philbert wrote:

 On 2016-04-06 10:24 AM, James Bottomley wrote:
> On Wed, 2016-04-06 at 10:11 -0400, Bastien Philbert wrote:
>>
>> On 2016-04-06 09:38 AM, James Bottomley wrote:
>>> On Wed, 2016-04-06 at 09:21 -0400, Bastien Philbert wrote:

 On 2016-04-06 03:48 AM, Julian Calaby wrote:
> Hi Bastien,
>
> On Wed, Apr 6, 2016 at 7:19 AM, Bastien Philbert
>  wrote:
>> This fixes backwards locking in the function
>> __csio_unreg_rnode
>> to
>> properly lock before the call to the function
>> csio_unreg_rnode
>> and
>> not unlock with spin_unlock_irq as this would not
>> allow
>> the
>> proper
>> protection for concurrent access on the shared
>> csio_hw
>> structure
>> pointer hw. In addition switch the locking after the
>> critical
>> region
>> function call to properly unlock instead with
>> spin_unlock_irq
>> on
>>
>> Signed-off-by: Bastien Philbert <
>> bastienphilb...@gmail.com>
>> ---
>>  drivers/scsi/csiostor/csio_rnode.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/csiostor/csio_rnode.c
>> b/drivers/scsi/csiostor/csio_rnode.c
>> index e9c3b04..029a09e 100644
>> --- a/drivers/scsi/csiostor/csio_rnode.c
>> +++ b/drivers/scsi/csiostor/csio_rnode.c
>> @@ -580,9 +580,9 @@ __csio_unreg_rnode(struct
>> csio_rnode
>> *rn)
>> ln->last_scan_ntgts--;
>> }
>>
>> -   spin_unlock_irq(>lock);
>> -   csio_unreg_rnode(rn);
>> spin_lock_irq(>lock);
>> +   csio_unreg_rnode(rn);
>> +   spin_unlock_irq(>lock);
>
> Are you _certain_ this is correct? This construct
> usually
> appears
> when
> a function has a particular lock held, then needs to
> unlock
> it
> to
> call
> some other function. Are you _certain_ that this isn't
> the
> case?
>
> Thanks,
>
 Yes I am pretty certain this is correct. I checked the
 paths
 that
 called this function
 and it was weired that none of them gradded the spinlock
 before
 hand.
>>>
>>> That's not good enough.  If your theory is correct, lockdep
>>> should
>>> be
>>> dropping an already unlocked assertion in this codepath ...
>>> do
>>> you
>>> see
>>> this?
>>>
>>> James
>>>
>>>
>> Yes I do.
>
> You mean you don't see the lockdep assert, since you're asking
> to 
> drop the patch?
>
>>  For now just drop the patch but I am still concerned that we
>> are
>> double unlocking here.
>
> Really, no.  The pattern in the code indicates the lock is
> expected 
> to be held.  This can be wrong (sometimes code moves or people
> forget), but if it is wrong we'll get an assert about unlock of
> an 
> already unlocked lock.  If there's no assert, the lock is held
> on 
> entry and the code is correct.
>
> You're proposing patches based on misunderstandings of the code
> which aren't backed up by actual issues and wasting everyone's
> time 
> to look at them.  Please begin with the hard evidence of a
> problem 
> first, so post the lockdep assert in the changelog so we know 
> there's a real problem.
>
> James
>
 Certainly James. I think I just got carried away with the last
 few 
 patches :(.
>>>
>>> Is this Nick Krause?  An email reply that Martin forwarded but the 
>>> list didn't pick up (because it had a html part) suggests this. 
>>>  What you're doing is what got you banned from LKML the last time: 
>>> sending patches without evidence there's a problem or understanding 
>>> the code you're patching.  Repeating the behaviour under a new 
>>> identity isn't going to help improve your standing.
>>>
>>> James
>>>
>> No I am not Nick Krause. I am just aware of how he got banned a few 
>> years ago. That email was a mistake by typo and was hoping nobody 
>> picked it up as they would then believe I was Nick Krause.
> 
> Hm, OK, but currently you are repeating his behaviour ... please don't
> send any more patches until they're about real problems backed by
> actual data.
> 
> Thanks,
> 
> James
> 
> 
Ok sure I do have one patch that I tested and it worked for me but wasn't sure 
if I am just
trampling over the 

Re: [PATCH] csiostor: Fix backwards locking in the function __csio_unreg_rnode

[James Bottomley]

On Wed, 2016-04-06 at 13:23 -0400, Bastien Philbert wrote:
> 
> On 2016-04-06 01:14 PM, James Bottomley wrote:
> > On Wed, 2016-04-06 at 10:36 -0400, Bastien Philbert wrote:
> > > 
> > > On 2016-04-06 10:24 AM, James Bottomley wrote:
> > > > On Wed, 2016-04-06 at 10:11 -0400, Bastien Philbert wrote:
> > > > > 
> > > > > On 2016-04-06 09:38 AM, James Bottomley wrote:
> > > > > > On Wed, 2016-04-06 at 09:21 -0400, Bastien Philbert wrote:
> > > > > > > 
> > > > > > > On 2016-04-06 03:48 AM, Julian Calaby wrote:
> > > > > > > > Hi Bastien,
> > > > > > > > 
> > > > > > > > On Wed, Apr 6, 2016 at 7:19 AM, Bastien Philbert
> > > > > > > >  wrote:
> > > > > > > > > This fixes backwards locking in the function
> > > > > > > > > __csio_unreg_rnode
> > > > > > > > > to
> > > > > > > > > properly lock before the call to the function
> > > > > > > > > csio_unreg_rnode
> > > > > > > > > and
> > > > > > > > > not unlock with spin_unlock_irq as this would not
> > > > > > > > > allow
> > > > > > > > > the
> > > > > > > > > proper
> > > > > > > > > protection for concurrent access on the shared
> > > > > > > > > csio_hw
> > > > > > > > > structure
> > > > > > > > > pointer hw. In addition switch the locking after the
> > > > > > > > > critical
> > > > > > > > > region
> > > > > > > > > function call to properly unlock instead with
> > > > > > > > > spin_unlock_irq
> > > > > > > > > on
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Bastien Philbert <
> > > > > > > > > bastienphilb...@gmail.com>
> > > > > > > > > ---
> > > > > > > > >  drivers/scsi/csiostor/csio_rnode.c | 4 ++--
> > > > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/scsi/csiostor/csio_rnode.c
> > > > > > > > > b/drivers/scsi/csiostor/csio_rnode.c
> > > > > > > > > index e9c3b04..029a09e 100644
> > > > > > > > > --- a/drivers/scsi/csiostor/csio_rnode.c
> > > > > > > > > +++ b/drivers/scsi/csiostor/csio_rnode.c
> > > > > > > > > @@ -580,9 +580,9 @@ __csio_unreg_rnode(struct
> > > > > > > > > csio_rnode
> > > > > > > > > *rn)
> > > > > > > > > ln->last_scan_ntgts--;
> > > > > > > > > }
> > > > > > > > > 
> > > > > > > > > -   spin_unlock_irq(>lock);
> > > > > > > > > -   csio_unreg_rnode(rn);
> > > > > > > > > spin_lock_irq(>lock);
> > > > > > > > > +   csio_unreg_rnode(rn);
> > > > > > > > > +   spin_unlock_irq(>lock);
> > > > > > > > 
> > > > > > > > Are you _certain_ this is correct? This construct
> > > > > > > > usually
> > > > > > > > appears
> > > > > > > > when
> > > > > > > > a function has a particular lock held, then needs to
> > > > > > > > unlock
> > > > > > > > it
> > > > > > > > to
> > > > > > > > call
> > > > > > > > some other function. Are you _certain_ that this isn't
> > > > > > > > the
> > > > > > > > case?
> > > > > > > > 
> > > > > > > > Thanks,
> > > > > > > > 
> > > > > > > Yes I am pretty certain this is correct. I checked the
> > > > > > > paths
> > > > > > > that
> > > > > > > called this function
> > > > > > > and it was weired that none of them gradded the spinlock
> > > > > > > before
> > > > > > > hand.
> > > > > > 
> > > > > > That's not good enough.  If your theory is correct, lockdep
> > > > > > should
> > > > > > be
> > > > > > dropping an already unlocked assertion in this codepath ...
> > > > > > do
> > > > > > you
> > > > > > see
> > > > > > this?
> > > > > > 
> > > > > > James
> > > > > > 
> > > > > > 
> > > > > Yes I do.
> > > > 
> > > > You mean you don't see the lockdep assert, since you're asking
> > > > to 
> > > > drop the patch?
> > > > 
> > > > >  For now just drop the patch but I am still concerned that we
> > > > > are
> > > > > double unlocking here.
> > > > 
> > > > Really, no.  The pattern in the code indicates the lock is
> > > > expected 
> > > > to be held.  This can be wrong (sometimes code moves or people
> > > > forget), but if it is wrong we'll get an assert about unlock of
> > > > an 
> > > > already unlocked lock.  If there's no assert, the lock is held
> > > > on 
> > > > entry and the code is correct.
> > > > 
> > > > You're proposing patches based on misunderstandings of the code
> > > > which aren't backed up by actual issues and wasting everyone's
> > > > time 
> > > > to look at them.  Please begin with the hard evidence of a
> > > > problem 
> > > > first, so post the lockdep assert in the changelog so we know 
> > > > there's a real problem.
> > > > 
> > > > James
> > > > 
> > > Certainly James. I think I just got carried away with the last
> > > few 
> > > patches :(.
> > 
> > Is this Nick Krause?  An email reply that Martin forwarded but the 
> > list didn't pick up (because it had a html part) suggests this. 
> >  What you're doing is what got you banned from LKML the last time: 
> > sending patches without evidence there's a problem or understanding 
> > the code you're patching.  Repeating the behaviour under a new 
> > identity isn't going 

Re: [PATCH] csiostor: Fix backwards locking in the function __csio_unreg_rnode

[Bastien Philbert]

On 2016-04-06 01:14 PM, James Bottomley wrote:
> On Wed, 2016-04-06 at 10:36 -0400, Bastien Philbert wrote:
>>
>> On 2016-04-06 10:24 AM, James Bottomley wrote:
>>> On Wed, 2016-04-06 at 10:11 -0400, Bastien Philbert wrote:

 On 2016-04-06 09:38 AM, James Bottomley wrote:
> On Wed, 2016-04-06 at 09:21 -0400, Bastien Philbert wrote:
>>
>> On 2016-04-06 03:48 AM, Julian Calaby wrote:
>>> Hi Bastien,
>>>
>>> On Wed, Apr 6, 2016 at 7:19 AM, Bastien Philbert
>>>  wrote:
 This fixes backwards locking in the function
 __csio_unreg_rnode
 to
 properly lock before the call to the function
 csio_unreg_rnode
 and
 not unlock with spin_unlock_irq as this would not allow
 the
 proper
 protection for concurrent access on the shared csio_hw
 structure
 pointer hw. In addition switch the locking after the
 critical
 region
 function call to properly unlock instead with
 spin_unlock_irq
 on

 Signed-off-by: Bastien Philbert <
 bastienphilb...@gmail.com>
 ---
  drivers/scsi/csiostor/csio_rnode.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

 diff --git a/drivers/scsi/csiostor/csio_rnode.c
 b/drivers/scsi/csiostor/csio_rnode.c
 index e9c3b04..029a09e 100644
 --- a/drivers/scsi/csiostor/csio_rnode.c
 +++ b/drivers/scsi/csiostor/csio_rnode.c
 @@ -580,9 +580,9 @@ __csio_unreg_rnode(struct csio_rnode
 *rn)
 ln->last_scan_ntgts--;
 }

 -   spin_unlock_irq(>lock);
 -   csio_unreg_rnode(rn);
 spin_lock_irq(>lock);
 +   csio_unreg_rnode(rn);
 +   spin_unlock_irq(>lock);
>>>
>>> Are you _certain_ this is correct? This construct usually
>>> appears
>>> when
>>> a function has a particular lock held, then needs to unlock
>>> it
>>> to
>>> call
>>> some other function. Are you _certain_ that this isn't the
>>> case?
>>>
>>> Thanks,
>>>
>> Yes I am pretty certain this is correct. I checked the paths
>> that
>> called this function
>> and it was weired that none of them gradded the spinlock
>> before
>> hand.
>
> That's not good enough.  If your theory is correct, lockdep
> should
> be
> dropping an already unlocked assertion in this codepath ... do
> you
> see
> this?
>
> James
>
>
 Yes I do.
>>>
>>> You mean you don't see the lockdep assert, since you're asking to 
>>> drop the patch?
>>>
  For now just drop the patch but I am still concerned that we are
 double unlocking here.
>>>
>>> Really, no.  The pattern in the code indicates the lock is expected 
>>> to be held.  This can be wrong (sometimes code moves or people
>>> forget), but if it is wrong we'll get an assert about unlock of an 
>>> already unlocked lock.  If there's no assert, the lock is held on 
>>> entry and the code is correct.
>>>
>>> You're proposing patches based on misunderstandings of the code 
>>> which aren't backed up by actual issues and wasting everyone's time 
>>> to look at them.  Please begin with the hard evidence of a problem 
>>> first, so post the lockdep assert in the changelog so we know 
>>> there's a real problem.
>>>
>>> James
>>>
>> Certainly James. I think I just got carried away with the last few 
>> patches :(.
> 
> Is this Nick Krause?  An email reply that Martin forwarded but the list
> didn't pick up (because it had a html part) suggests this.  What you're
> doing is what got you banned from LKML the last time: sending patches
> without evidence there's a problem or understanding the code you're
> patching.  Repeating the behaviour under a new identity isn't going to
> help improve your standing.
> 
> James
> 
No I am not Nick Krause. I am just aware of how he got banned a few years ago.
That email was a mistake by typo and was hoping nobody picked it up as they 
would then believe I was Nick Krause.
Bastien
--
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] csiostor: Fix backwards locking in the function __csio_unreg_rnode

[James Bottomley]

On Wed, 2016-04-06 at 10:36 -0400, Bastien Philbert wrote:
> 
> On 2016-04-06 10:24 AM, James Bottomley wrote:
> > On Wed, 2016-04-06 at 10:11 -0400, Bastien Philbert wrote:
> > > 
> > > On 2016-04-06 09:38 AM, James Bottomley wrote:
> > > > On Wed, 2016-04-06 at 09:21 -0400, Bastien Philbert wrote:
> > > > > 
> > > > > On 2016-04-06 03:48 AM, Julian Calaby wrote:
> > > > > > Hi Bastien,
> > > > > > 
> > > > > > On Wed, Apr 6, 2016 at 7:19 AM, Bastien Philbert
> > > > > >  wrote:
> > > > > > > This fixes backwards locking in the function
> > > > > > > __csio_unreg_rnode
> > > > > > > to
> > > > > > > properly lock before the call to the function
> > > > > > > csio_unreg_rnode
> > > > > > > and
> > > > > > > not unlock with spin_unlock_irq as this would not allow
> > > > > > > the
> > > > > > > proper
> > > > > > > protection for concurrent access on the shared csio_hw
> > > > > > > structure
> > > > > > > pointer hw. In addition switch the locking after the
> > > > > > > critical
> > > > > > > region
> > > > > > > function call to properly unlock instead with
> > > > > > > spin_unlock_irq
> > > > > > > on
> > > > > > > 
> > > > > > > Signed-off-by: Bastien Philbert <
> > > > > > > bastienphilb...@gmail.com>
> > > > > > > ---
> > > > > > >  drivers/scsi/csiostor/csio_rnode.c | 4 ++--
> > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/scsi/csiostor/csio_rnode.c
> > > > > > > b/drivers/scsi/csiostor/csio_rnode.c
> > > > > > > index e9c3b04..029a09e 100644
> > > > > > > --- a/drivers/scsi/csiostor/csio_rnode.c
> > > > > > > +++ b/drivers/scsi/csiostor/csio_rnode.c
> > > > > > > @@ -580,9 +580,9 @@ __csio_unreg_rnode(struct csio_rnode
> > > > > > > *rn)
> > > > > > > ln->last_scan_ntgts--;
> > > > > > > }
> > > > > > > 
> > > > > > > -   spin_unlock_irq(>lock);
> > > > > > > -   csio_unreg_rnode(rn);
> > > > > > > spin_lock_irq(>lock);
> > > > > > > +   csio_unreg_rnode(rn);
> > > > > > > +   spin_unlock_irq(>lock);
> > > > > > 
> > > > > > Are you _certain_ this is correct? This construct usually
> > > > > > appears
> > > > > > when
> > > > > > a function has a particular lock held, then needs to unlock
> > > > > > it
> > > > > > to
> > > > > > call
> > > > > > some other function. Are you _certain_ that this isn't the
> > > > > > case?
> > > > > > 
> > > > > > Thanks,
> > > > > > 
> > > > > Yes I am pretty certain this is correct. I checked the paths
> > > > > that
> > > > > called this function
> > > > > and it was weired that none of them gradded the spinlock
> > > > > before
> > > > > hand.
> > > > 
> > > > That's not good enough.  If your theory is correct, lockdep
> > > > should
> > > > be
> > > > dropping an already unlocked assertion in this codepath ... do
> > > > you
> > > > see
> > > > this?
> > > > 
> > > > James
> > > > 
> > > > 
> > > Yes I do.
> > 
> > You mean you don't see the lockdep assert, since you're asking to 
> > drop the patch?
> > 
> > >  For now just drop the patch but I am still concerned that we are
> > > double unlocking here.
> > 
> > Really, no.  The pattern in the code indicates the lock is expected 
> > to be held.  This can be wrong (sometimes code moves or people
> > forget), but if it is wrong we'll get an assert about unlock of an 
> > already unlocked lock.  If there's no assert, the lock is held on 
> > entry and the code is correct.
> > 
> > You're proposing patches based on misunderstandings of the code 
> > which aren't backed up by actual issues and wasting everyone's time 
> > to look at them.  Please begin with the hard evidence of a problem 
> > first, so post the lockdep assert in the changelog so we know 
> > there's a real problem.
> > 
> > James
> > 
> Certainly James. I think I just got carried away with the last few 
> patches :(.

Is this Nick Krause?  An email reply that Martin forwarded but the list
didn't pick up (because it had a html part) suggests this.  What you're
doing is what got you banned from LKML the last time: sending patches
without evidence there's a problem or understanding the code you're
patching.  Repeating the behaviour under a new identity isn't going to
help improve your standing.

James

--
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] csiostor: Fix backwards locking in the function __csio_unreg_rnode

[Bastien Philbert]

On 2016-04-06 10:24 AM, James Bottomley wrote:
> On Wed, 2016-04-06 at 10:11 -0400, Bastien Philbert wrote:
>>
>> On 2016-04-06 09:38 AM, James Bottomley wrote:
>>> On Wed, 2016-04-06 at 09:21 -0400, Bastien Philbert wrote:

 On 2016-04-06 03:48 AM, Julian Calaby wrote:
> Hi Bastien,
>
> On Wed, Apr 6, 2016 at 7:19 AM, Bastien Philbert
>  wrote:
>> This fixes backwards locking in the function
>> __csio_unreg_rnode
>> to
>> properly lock before the call to the function
>> csio_unreg_rnode
>> and
>> not unlock with spin_unlock_irq as this would not allow the
>> proper
>> protection for concurrent access on the shared csio_hw
>> structure
>> pointer hw. In addition switch the locking after the critical
>> region
>> function call to properly unlock instead with spin_unlock_irq
>> on
>>
>> Signed-off-by: Bastien Philbert 
>> ---
>>  drivers/scsi/csiostor/csio_rnode.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/csiostor/csio_rnode.c
>> b/drivers/scsi/csiostor/csio_rnode.c
>> index e9c3b04..029a09e 100644
>> --- a/drivers/scsi/csiostor/csio_rnode.c
>> +++ b/drivers/scsi/csiostor/csio_rnode.c
>> @@ -580,9 +580,9 @@ __csio_unreg_rnode(struct csio_rnode *rn)
>> ln->last_scan_ntgts--;
>> }
>>
>> -   spin_unlock_irq(>lock);
>> -   csio_unreg_rnode(rn);
>> spin_lock_irq(>lock);
>> +   csio_unreg_rnode(rn);
>> +   spin_unlock_irq(>lock);
>
> Are you _certain_ this is correct? This construct usually
> appears
> when
> a function has a particular lock held, then needs to unlock it
> to
> call
> some other function. Are you _certain_ that this isn't the
> case?
>
> Thanks,
>
 Yes I am pretty certain this is correct. I checked the paths that
 called this function
 and it was weired that none of them gradded the spinlock before
 hand.
>>>
>>> That's not good enough.  If your theory is correct, lockdep should
>>> be
>>> dropping an already unlocked assertion in this codepath ... do you
>>> see
>>> this?
>>>
>>> James
>>>
>>>
>> Yes I do.
> 
> You mean you don't see the lockdep assert, since you're asking to drop
> the patch?
> 
>>  For now just drop the patch but I am still concerned that we are
>> double unlocking here.
> 
> Really, no.  The pattern in the code indicates the lock is expected to
> be held.  This can be wrong (sometimes code moves or people forget),
> but if it is wrong we'll get an assert about unlock of an already
> unlocked lock.  If there's no assert, the lock is held on entry and the
> code is correct.
> 
> You're proposing patches based on misunderstandings of the code which
> aren't backed up by actual issues and wasting everyone's time to look
> at them.  Please begin with the hard evidence of a problem first, so
> post the lockdep assert in the changelog so we know there's a real
> problem.
> 
> James
> 
Certainly James. I think I just got carried away with the last few patches :(.
Bastien
--
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] csiostor: Fix backwards locking in the function __csio_unreg_rnode

[James Bottomley]

On Wed, 2016-04-06 at 10:11 -0400, Bastien Philbert wrote:
> 
> On 2016-04-06 09:38 AM, James Bottomley wrote:
> > On Wed, 2016-04-06 at 09:21 -0400, Bastien Philbert wrote:
> > > 
> > > On 2016-04-06 03:48 AM, Julian Calaby wrote:
> > > > Hi Bastien,
> > > > 
> > > > On Wed, Apr 6, 2016 at 7:19 AM, Bastien Philbert
> > > >  wrote:
> > > > > This fixes backwards locking in the function
> > > > > __csio_unreg_rnode
> > > > > to
> > > > > properly lock before the call to the function
> > > > > csio_unreg_rnode
> > > > > and
> > > > > not unlock with spin_unlock_irq as this would not allow the
> > > > > proper
> > > > > protection for concurrent access on the shared csio_hw
> > > > > structure
> > > > > pointer hw. In addition switch the locking after the critical
> > > > > region
> > > > > function call to properly unlock instead with spin_unlock_irq
> > > > > on
> > > > > 
> > > > > Signed-off-by: Bastien Philbert 
> > > > > ---
> > > > >  drivers/scsi/csiostor/csio_rnode.c | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/scsi/csiostor/csio_rnode.c
> > > > > b/drivers/scsi/csiostor/csio_rnode.c
> > > > > index e9c3b04..029a09e 100644
> > > > > --- a/drivers/scsi/csiostor/csio_rnode.c
> > > > > +++ b/drivers/scsi/csiostor/csio_rnode.c
> > > > > @@ -580,9 +580,9 @@ __csio_unreg_rnode(struct csio_rnode *rn)
> > > > > ln->last_scan_ntgts--;
> > > > > }
> > > > > 
> > > > > -   spin_unlock_irq(>lock);
> > > > > -   csio_unreg_rnode(rn);
> > > > > spin_lock_irq(>lock);
> > > > > +   csio_unreg_rnode(rn);
> > > > > +   spin_unlock_irq(>lock);
> > > > 
> > > > Are you _certain_ this is correct? This construct usually
> > > > appears
> > > > when
> > > > a function has a particular lock held, then needs to unlock it
> > > > to
> > > > call
> > > > some other function. Are you _certain_ that this isn't the
> > > > case?
> > > > 
> > > > Thanks,
> > > > 
> > > Yes I am pretty certain this is correct. I checked the paths that
> > > called this function
> > > and it was weired that none of them gradded the spinlock before
> > > hand.
> > 
> > That's not good enough.  If your theory is correct, lockdep should
> > be
> > dropping an already unlocked assertion in this codepath ... do you
> > see
> > this?
> > 
> > James
> > 
> > 
> Yes I do.

You mean you don't see the lockdep assert, since you're asking to drop
the patch?

>  For now just drop the patch but I am still concerned that we are
> double unlocking here.

Really, no.  The pattern in the code indicates the lock is expected to
be held.  This can be wrong (sometimes code moves or people forget),
but if it is wrong we'll get an assert about unlock of an already
unlocked lock.  If there's no assert, the lock is held on entry and the
code is correct.

You're proposing patches based on misunderstandings of the code which
aren't backed up by actual issues and wasting everyone's time to look
at them.  Please begin with the hard evidence of a problem first, so
post the lockdep assert in the changelog so we know there's a real
problem.

James

--
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] csiostor: Fix backwards locking in the function __csio_unreg_rnode

[Bastien Philbert]

On 2016-04-06 09:38 AM, James Bottomley wrote:
> On Wed, 2016-04-06 at 09:21 -0400, Bastien Philbert wrote:
>>
>> On 2016-04-06 03:48 AM, Julian Calaby wrote:
>>> Hi Bastien,
>>>
>>> On Wed, Apr 6, 2016 at 7:19 AM, Bastien Philbert
>>>  wrote:
 This fixes backwards locking in the function __csio_unreg_rnode
 to
 properly lock before the call to the function csio_unreg_rnode
 and
 not unlock with spin_unlock_irq as this would not allow the
 proper
 protection for concurrent access on the shared csio_hw structure
 pointer hw. In addition switch the locking after the critical
 region
 function call to properly unlock instead with spin_unlock_irq on

 Signed-off-by: Bastien Philbert 
 ---
  drivers/scsi/csiostor/csio_rnode.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

 diff --git a/drivers/scsi/csiostor/csio_rnode.c
 b/drivers/scsi/csiostor/csio_rnode.c
 index e9c3b04..029a09e 100644
 --- a/drivers/scsi/csiostor/csio_rnode.c
 +++ b/drivers/scsi/csiostor/csio_rnode.c
 @@ -580,9 +580,9 @@ __csio_unreg_rnode(struct csio_rnode *rn)
 ln->last_scan_ntgts--;
 }

 -   spin_unlock_irq(>lock);
 -   csio_unreg_rnode(rn);
 spin_lock_irq(>lock);
 +   csio_unreg_rnode(rn);
 +   spin_unlock_irq(>lock);
>>>
>>> Are you _certain_ this is correct? This construct usually appears
>>> when
>>> a function has a particular lock held, then needs to unlock it to
>>> call
>>> some other function. Are you _certain_ that this isn't the case?
>>>
>>> Thanks,
>>>
>> Yes I am pretty certain this is correct. I checked the paths that
>> called this function
>> and it was weired that none of them gradded the spinlock before hand.
> 
> That's not good enough.  If your theory is correct, lockdep should be
> dropping an already unlocked assertion in this codepath ... do you see
> this?
> 
> James
> 
> 
Yes I do. For now just drop the patch but I am still concerned that we are 
double unlocking here.
Bastien
> 
> 
--
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] csiostor: Fix backwards locking in the function __csio_unreg_rnode

[James Bottomley]

On Wed, 2016-04-06 at 09:21 -0400, Bastien Philbert wrote:
> 
> On 2016-04-06 03:48 AM, Julian Calaby wrote:
> > Hi Bastien,
> > 
> > On Wed, Apr 6, 2016 at 7:19 AM, Bastien Philbert
> >  wrote:
> > > This fixes backwards locking in the function __csio_unreg_rnode
> > > to
> > > properly lock before the call to the function csio_unreg_rnode
> > > and
> > > not unlock with spin_unlock_irq as this would not allow the
> > > proper
> > > protection for concurrent access on the shared csio_hw structure
> > > pointer hw. In addition switch the locking after the critical
> > > region
> > > function call to properly unlock instead with spin_unlock_irq on
> > > 
> > > Signed-off-by: Bastien Philbert 
> > > ---
> > >  drivers/scsi/csiostor/csio_rnode.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/csiostor/csio_rnode.c
> > > b/drivers/scsi/csiostor/csio_rnode.c
> > > index e9c3b04..029a09e 100644
> > > --- a/drivers/scsi/csiostor/csio_rnode.c
> > > +++ b/drivers/scsi/csiostor/csio_rnode.c
> > > @@ -580,9 +580,9 @@ __csio_unreg_rnode(struct csio_rnode *rn)
> > > ln->last_scan_ntgts--;
> > > }
> > > 
> > > -   spin_unlock_irq(>lock);
> > > -   csio_unreg_rnode(rn);
> > > spin_lock_irq(>lock);
> > > +   csio_unreg_rnode(rn);
> > > +   spin_unlock_irq(>lock);
> > 
> > Are you _certain_ this is correct? This construct usually appears
> > when
> > a function has a particular lock held, then needs to unlock it to
> > call
> > some other function. Are you _certain_ that this isn't the case?
> > 
> > Thanks,
> > 
> Yes I am pretty certain this is correct. I checked the paths that
> called this function
> and it was weired that none of them gradded the spinlock before hand.

That's not good enough.  If your theory is correct, lockdep should be
dropping an already unlocked assertion in this codepath ... do you see
this?

James




--
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] csiostor: Fix backwards locking in the function __csio_unreg_rnode

[Bastien Philbert]

On 2016-04-06 03:48 AM, Julian Calaby wrote:
> Hi Bastien,
> 
> On Wed, Apr 6, 2016 at 7:19 AM, Bastien Philbert
>  wrote:
>> This fixes backwards locking in the function __csio_unreg_rnode to
>> properly lock before the call to the function csio_unreg_rnode and
>> not unlock with spin_unlock_irq as this would not allow the proper
>> protection for concurrent access on the shared csio_hw structure
>> pointer hw. In addition switch the locking after the critical region
>> function call to properly unlock instead with spin_unlock_irq on
>>
>> Signed-off-by: Bastien Philbert 
>> ---
>>  drivers/scsi/csiostor/csio_rnode.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/csiostor/csio_rnode.c 
>> b/drivers/scsi/csiostor/csio_rnode.c
>> index e9c3b04..029a09e 100644
>> --- a/drivers/scsi/csiostor/csio_rnode.c
>> +++ b/drivers/scsi/csiostor/csio_rnode.c
>> @@ -580,9 +580,9 @@ __csio_unreg_rnode(struct csio_rnode *rn)
>> ln->last_scan_ntgts--;
>> }
>>
>> -   spin_unlock_irq(>lock);
>> -   csio_unreg_rnode(rn);
>> spin_lock_irq(>lock);
>> +   csio_unreg_rnode(rn);
>> +   spin_unlock_irq(>lock);
> 
> Are you _certain_ this is correct? This construct usually appears when
> a function has a particular lock held, then needs to unlock it to call
> some other function. Are you _certain_ that this isn't the case?
> 
> Thanks,
> 
Yes I am pretty certain this is correct. I checked the paths that called this 
function
and it was weired that none of them gradded the spinlock before hand.
Cheers,
Bastien
--
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 19/30] scsi: use parity32 in isci's phy

[zengzhaoxiu]

From: Zhaoxiu Zeng 

Signed-off-by: Zhaoxiu Zeng 
---
 drivers/scsi/isci/phy.c | 15 +--
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/scsi/isci/phy.c b/drivers/scsi/isci/phy.c
index cb87b2e..a06aff6 100644
--- a/drivers/scsi/isci/phy.c
+++ b/drivers/scsi/isci/phy.c
@@ -122,8 +122,6 @@ sci_phy_link_layer_initialization(struct isci_phy *iphy,
int phy_idx = iphy->phy_index;
struct sci_phy_cap phy_cap;
u32 phy_configuration;
-   u32 parity_check = 0;
-   u32 parity_count = 0;
u32 llctl, link_rate;
u32 clksm_value = 0;
u32 sp_timeouts = 0;
@@ -225,18 +223,7 @@ sci_phy_link_layer_initialization(struct isci_phy *iphy,
/* The SAS specification indicates that the phy_capabilities that
 * are transmitted shall have an even parity.  Calculate the parity.
 */
-   parity_check = phy_cap.all;
-   while (parity_check != 0) {
-   if (parity_check & 0x1)
-   parity_count++;
-   parity_check >>= 1;
-   }
-
-   /* If parity indicates there are an odd number of bits set, then
-* set the parity bit to 1 in the phy capabilities.
-*/
-   if ((parity_count % 2) != 0)
-   phy_cap.parity = 1;
+   phy_cap.parity = parity32(phy_cap.all);
 
writel(phy_cap.all, >phy_capabilities);
 
-- 
2.5.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

Re: [PATCH v4 0/2] Update SCSI target removal path

[Johannes Thumshirn]

On 2016-04-05 13:18, Martin K. Petersen wrote:

"Johannes" == Johannes Thumshirn  writes:


Johannes> This is a follow up to "scsi: Add intermediate STARGET_REMOVE
Johannes> state to scsi_target_state".

James suggested we should let this incubate before hitting stable.

Dropped v3 from 4.6/scsi-fixes and applied v4 to 4.7/scsi-queue.


Sounds good to me.
--
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 8/8] scsi: ufs: connect to RPMB subsystem

[Joao Pinto]

Hi!

On 4/4/2016 12:11 PM, Tomas Winkler wrote:
> Register UFS RPMB LUN with the RPMB subsystem and provide
> implementation for the RPMB access operations. RPMB partition is
> accessed via a sequence of security protocol in and security protocol
> out commands with UFS specific parameters. This multi step process is
> abstracted into 4 basic RPMB commands.

[snip]

>* "UFS device" W-LU.
>*/
>   struct scsi_device *sdev_ufs_device;
> + struct scsi_device *sdev_ufs_rpmb;
>  
>   enum ufs_dev_pwr_mode curr_dev_pwr_mode;
>   enum uic_link_state uic_link_state;
> 

I have a UFS device emulator that has the RPMB capability. What are the expected
good results for me to validate?

Thanks.
--
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/4] libata: implement ZBC IN translation

[Hannes Reinecke]

On 04/04/2016 05:49 PM, Tejun Heo wrote:
> On Mon, Apr 04, 2016 at 11:47:48AM +0200, Hannes Reinecke wrote:
>> +/*
> 
> /**
> 
>> + * ata_scsi_report_zones_complete
>> + *
>> + * Convert T-13 little-endian field representation into
>> + * T-10 big-endian field representation.
>> + */
>> +static void ata_scsi_report_zones_complete(struct ata_queued_cmd *qc)
>> +{
>> +struct scsi_cmnd *scmd = qc->scsicmd;
>> +struct sg_mapping_iter miter;
>> +unsigned long flags;
>> +unsigned int bytes = 0;
>> +
>> +sg_miter_start(, scsi_sglist(scmd), scsi_sg_count(scmd),
>> +   SG_MITER_TO_SG | SG_MITER_ATOMIC);
>> +
>> +local_irq_save(flags);
>> +while (sg_miter_next()) {
>> +unsigned int offset = 0;
> 
> Not this patch's fault but ugh  :(
> 
I know. I've been asking the very same questions to T-13.
But apparently it's by design ... gnaa.

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] csiostor: Fix backwards locking in the function __csio_unreg_rnode

[Julian Calaby]

Hi Bastien,

On Wed, Apr 6, 2016 at 7:19 AM, Bastien Philbert
 wrote:
> This fixes backwards locking in the function __csio_unreg_rnode to
> properly lock before the call to the function csio_unreg_rnode and
> not unlock with spin_unlock_irq as this would not allow the proper
> protection for concurrent access on the shared csio_hw structure
> pointer hw. In addition switch the locking after the critical region
> function call to properly unlock instead with spin_unlock_irq on
>
> Signed-off-by: Bastien Philbert 
> ---
>  drivers/scsi/csiostor/csio_rnode.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/csiostor/csio_rnode.c 
> b/drivers/scsi/csiostor/csio_rnode.c
> index e9c3b04..029a09e 100644
> --- a/drivers/scsi/csiostor/csio_rnode.c
> +++ b/drivers/scsi/csiostor/csio_rnode.c
> @@ -580,9 +580,9 @@ __csio_unreg_rnode(struct csio_rnode *rn)
> ln->last_scan_ntgts--;
> }
>
> -   spin_unlock_irq(>lock);
> -   csio_unreg_rnode(rn);
> spin_lock_irq(>lock);
> +   csio_unreg_rnode(rn);
> +   spin_unlock_irq(>lock);

Are you _certain_ this is correct? This construct usually appears when
a function has a particular lock held, then needs to unlock it to call
some other function. Are you _certain_ that this isn't the case?

Thanks,

-- 
Julian Calaby

Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
--
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