| Message ID | 20200124101311.drryaegcdc7d4x7e@kili.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-buf: fix locking in sync_print_obj() | expand |
Quoting Dan Carpenter (2020-01-24 10:13:12) > This is always called with IRQs disabled and we don't actually want to > enable IRQs at the end. > > Fixes: a6aa8fca4d79 ("dma-buf/sw-sync: Reduce irqsave/irqrestore from known context") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > drivers/dma-buf/sync_debug.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c > index 101394f16930..952331344b1c 100644 > --- a/drivers/dma-buf/sync_debug.c > +++ b/drivers/dma-buf/sync_debug.c > @@ -107,15 +107,16 @@ static void sync_print_fence(struct seq_file *s, > static void sync_print_obj(struct seq_file *s, struct sync_timeline *obj) > { > struct list_head *pos; > + unsigned long flags; > > seq_printf(s, "%s: %d\n", obj->name, obj->value); > > - spin_lock_irq(&obj->lock); > + spin_lock_irqsave(&obj->lock, flags); Exactly, it can be just spin_lock() as the irq state is known. Once again I question why this [sync_debug.c] code even exists. -Chris
On Fri, Jan 24, 2020 at 10:20:56AM +0000, Chris Wilson wrote: > Quoting Dan Carpenter (2020-01-24 10:13:12) > > This is always called with IRQs disabled and we don't actually want to > > enable IRQs at the end. > > > > Fixes: a6aa8fca4d79 ("dma-buf/sw-sync: Reduce irqsave/irqrestore from known context") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > drivers/dma-buf/sync_debug.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c > > index 101394f16930..952331344b1c 100644 > > --- a/drivers/dma-buf/sync_debug.c > > +++ b/drivers/dma-buf/sync_debug.c > > @@ -107,15 +107,16 @@ static void sync_print_fence(struct seq_file *s, > > static void sync_print_obj(struct seq_file *s, struct sync_timeline *obj) > > { > > struct list_head *pos; > > + unsigned long flags; > > > > seq_printf(s, "%s: %d\n", obj->name, obj->value); > > > > - spin_lock_irq(&obj->lock); > > + spin_lock_irqsave(&obj->lock, flags); > > Exactly, it can be just spin_lock() as the irq state is known. > I did consider that but I wasn't sure how this is going to be used in the future so I took a conservative approach. > Once again I question why this [sync_debug.c] code even exists. No idea. regards, dan carpenter
Quoting Dan Carpenter (2020-01-24 10:31:23) > On Fri, Jan 24, 2020 at 10:20:56AM +0000, Chris Wilson wrote: > > Quoting Dan Carpenter (2020-01-24 10:13:12) > > > This is always called with IRQs disabled and we don't actually want to > > > enable IRQs at the end. > > > > > > Fixes: a6aa8fca4d79 ("dma-buf/sw-sync: Reduce irqsave/irqrestore from known context") > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > --- > > > drivers/dma-buf/sync_debug.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c > > > index 101394f16930..952331344b1c 100644 > > > --- a/drivers/dma-buf/sync_debug.c > > > +++ b/drivers/dma-buf/sync_debug.c > > > @@ -107,15 +107,16 @@ static void sync_print_fence(struct seq_file *s, > > > static void sync_print_obj(struct seq_file *s, struct sync_timeline *obj) > > > { > > > struct list_head *pos; > > > + unsigned long flags; > > > > > > seq_printf(s, "%s: %d\n", obj->name, obj->value); > > > > > > - spin_lock_irq(&obj->lock); > > > + spin_lock_irqsave(&obj->lock, flags); > > > > Exactly, it can be just spin_lock() as the irq state is known. > > > > I did consider that but I wasn't sure how this is going to be used in > the future so I took a conservative approach. Sure, it's debug so not critical (lists within lists to a seqfile, ouch) Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c index 101394f16930..952331344b1c 100644 --- a/drivers/dma-buf/sync_debug.c +++ b/drivers/dma-buf/sync_debug.c @@ -107,15 +107,16 @@ static void sync_print_fence(struct seq_file *s, static void sync_print_obj(struct seq_file *s, struct sync_timeline *obj) { struct list_head *pos; + unsigned long flags; seq_printf(s, "%s: %d\n", obj->name, obj->value); - spin_lock_irq(&obj->lock); + spin_lock_irqsave(&obj->lock, flags); list_for_each(pos, &obj->pt_list) { struct sync_pt *pt = container_of(pos, struct sync_pt, link); sync_print_fence(s, &pt->base, false); } - spin_unlock_irq(&obj->lock); + spin_unlock_irqrestore(&obj->lock, flags); } static void sync_print_sync_file(struct seq_file *s,
This is always called with IRQs disabled and we don't actually want to enable IRQs at the end. Fixes: a6aa8fca4d79 ("dma-buf/sw-sync: Reduce irqsave/irqrestore from known context") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/dma-buf/sync_debug.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)