diff mbox series

pci/switchtec: fix stream_open.cocci warnings (fwd)

Message ID alpine.DEB.2.21.1904131849350.2536@hadrien (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show
Series pci/switchtec: fix stream_open.cocci warnings (fwd) | expand

Commit Message

Julia Lawall April 13, 2019, 4:50 p.m. UTC
Hello,

Kirill will explain about this issue.

julia

---------- Forwarded message ----------
Date: Sat, 13 Apr 2019 11:22:51 +0800
From: kbuild test robot <lkp@intel.com>
To: kbuild@01.org
Cc: Julia Lawall <julia.lawall@lip6.fr>
Subject: [PATCH] pci/switchtec: fix stream_open.cocci warnings

CC: kbuild-all@01.org
TO: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
CC: Kurt Schwemmer <kurt.schwemmer@microsemi.com>
CC: Logan Gunthorpe <logang@deltatee.com>
CC: Bjorn Helgaas <helgaas@kernel.org>
CC: linux-pci@vger.kernel.org
CC: linux-kernel@vger.kernel.org

From: kbuild test robot <lkp@intel.com>

drivers/pci/switch/switchtec.c:395:1-17: ERROR: switchtec_fops: .read() can deadlock .write(); change nonseekable_open -> stream_open to fix.

Generated by: scripts/coccinelle/api/stream_open.cocci

Fixes: 8a29a3bae2a2 ("pci/switchtec: Don't use completion's wait queue")
Signed-off-by: kbuild test robot <lkp@intel.com>
---

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git linux-5.0.y-rt-rebase
head:   794c294ae4483c240429c25a0d18e272e92c94de
commit: 8a29a3bae2a2dfb0116cd8791d9700515d6e765e [154/311] pci/switchtec: Don't use completion's wait queue
:::::: branch date: 7 hours ago
:::::: commit date: 7 hours ago

Please take the patch only if it's a positive warning. Thanks!

 switchtec.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Logan Gunthorpe April 13, 2019, 4:56 p.m. UTC | #1
On 2019-04-13 10:50 a.m., Julia Lawall wrote:
> Hello,
> 
> Kirill will explain about this issue.

I'm aware of this effort and Acked Kirill's original monolithic patch.
So, for this patch:

Acked-by: Logan Gunthorpe <logang@deltatee.com.

Thanks,

Logan


> julia
> 
> ---------- Forwarded message ----------
> Date: Sat, 13 Apr 2019 11:22:51 +0800
> From: kbuild test robot <lkp@intel.com>
> To: kbuild@01.org
> Cc: Julia Lawall <julia.lawall@lip6.fr>
> Subject: [PATCH] pci/switchtec: fix stream_open.cocci warnings
> 
> CC: kbuild-all@01.org
> TO: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> CC: Kurt Schwemmer <kurt.schwemmer@microsemi.com>
> CC: Logan Gunthorpe <logang@deltatee.com>
> CC: Bjorn Helgaas <helgaas@kernel.org>
> CC: linux-pci@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> 
> From: kbuild test robot <lkp@intel.com>
> 
> drivers/pci/switch/switchtec.c:395:1-17: ERROR: switchtec_fops: .read() can deadlock .write(); change nonseekable_open -> stream_open to fix.
> 
> Generated by: scripts/coccinelle/api/stream_open.cocci
> 
> Fixes: 8a29a3bae2a2 ("pci/switchtec: Don't use completion's wait queue")
> Signed-off-by: kbuild test robot <lkp@intel.com>
> ---
> 
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git linux-5.0.y-rt-rebase
> head:   794c294ae4483c240429c25a0d18e272e92c94de
> commit: 8a29a3bae2a2dfb0116cd8791d9700515d6e765e [154/311] pci/switchtec: Don't use completion's wait queue
> :::::: branch date: 7 hours ago
> :::::: commit date: 7 hours ago
> 
> Please take the patch only if it's a positive warning. Thanks!
> 
>  switchtec.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/drivers/pci/switch/switchtec.c
> +++ b/drivers/pci/switch/switchtec.c
> @@ -392,7 +392,7 @@ static int switchtec_dev_open(struct ino
>  		return PTR_ERR(stuser);
> 
>  	filp->private_data = stuser;
> -	nonseekable_open(inode, filp);
> +	stream_open(inode, filp);
> 
>  	dev_dbg(&stdev->dev, "%s: %p\n", __func__, stuser);
>
Kirill Smelkov April 13, 2019, 5:01 p.m. UTC | #2
Hello everyone,

On Sat, Apr 13, 2019 at 06:50:57PM +0200, Julia Lawall wrote:
> Hello,
> 
> Kirill will explain about this issue.

pci/switchtec switching to stream_open is already queued to merge
window and it was acked by Logan Gunthorpe:

        https://lore.kernel.org/lkml/CAHk-=wgqgN5j1ZWnyVLqqoyU=CCWTYOko3MDyU8L_5e21KvHAg@mail.gmail.com/
        https://lab.nexedi.com/kirr/linux/commit/edaeb4101860

( there are too many Cc's in that patch and email with it and reply-all to
  it did not get into mailing list probably due to being considered as spam )

stream_open.cocci was issuing only warning for pci/switchtec, but after
8a29a3bae2a2 ("pci/switchtec: Don't use completion's wait queue") they
started to use wait_even_* inside read method and, since
stream_open.cocci considers wait_event_* as blocking the warning became
error. Previously it was completions there, but I added support for wait
events only for simplicity.

I can handle pci/switchtec switching via big nonseekable_open ->
stream_open change at next merge window, or, alternatively please feel
free to switch pci/switchtec now on its own. The change is correct - I
was manually reviewing all changes that stream_open.cocci produces and
pci/switchtec was there.

Kirill

> julia
> 
> ---------- Forwarded message ----------
> Date: Sat, 13 Apr 2019 11:22:51 +0800
> From: kbuild test robot <lkp@intel.com>
> To: kbuild@01.org
> Cc: Julia Lawall <julia.lawall@lip6.fr>
> Subject: [PATCH] pci/switchtec: fix stream_open.cocci warnings
> 
> CC: kbuild-all@01.org
> TO: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> CC: Kurt Schwemmer <kurt.schwemmer@microsemi.com>
> CC: Logan Gunthorpe <logang@deltatee.com>
> CC: Bjorn Helgaas <helgaas@kernel.org>
> CC: linux-pci@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> 
> From: kbuild test robot <lkp@intel.com>
> 
> drivers/pci/switch/switchtec.c:395:1-17: ERROR: switchtec_fops: .read() can deadlock .write(); change nonseekable_open -> stream_open to fix.
> 
> Generated by: scripts/coccinelle/api/stream_open.cocci
> 
> Fixes: 8a29a3bae2a2 ("pci/switchtec: Don't use completion's wait queue")
> Signed-off-by: kbuild test robot <lkp@intel.com>
> ---
> 
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git linux-5.0.y-rt-rebase
> head:   794c294ae4483c240429c25a0d18e272e92c94de
> commit: 8a29a3bae2a2dfb0116cd8791d9700515d6e765e [154/311] pci/switchtec: Don't use completion's wait queue
> :::::: branch date: 7 hours ago
> :::::: commit date: 7 hours ago
> 
> Please take the patch only if it's a positive warning. Thanks!
> 
>  switchtec.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/drivers/pci/switch/switchtec.c
> +++ b/drivers/pci/switch/switchtec.c
> @@ -392,7 +392,7 @@ static int switchtec_dev_open(struct ino
>  		return PTR_ERR(stuser);
> 
>  	filp->private_data = stuser;
> -	nonseekable_open(inode, filp);
> +	stream_open(inode, filp);
> 
>  	dev_dbg(&stdev->dev, "%s: %p\n", __func__, stuser);
Sebastian Andrzej Siewior April 15, 2019, 2:38 p.m. UTC | #3
On 2019-04-13 17:00:59 [+0000], Kirill Smelkov wrote:
> Hello everyone,
Hi,

> On Sat, Apr 13, 2019 at 06:50:57PM +0200, Julia Lawall wrote:
> > Hello,
> > 
> > Kirill will explain about this issue.
> 
> pci/switchtec switching to stream_open is already queued to merge
> window and it was acked by Logan Gunthorpe:
> 
>         https://lore.kernel.org/lkml/CAHk-=wgqgN5j1ZWnyVLqqoyU=CCWTYOko3MDyU8L_5e21KvHAg@mail.gmail.com/
>         https://lab.nexedi.com/kirr/linux/commit/edaeb4101860
> 
> ( there are too many Cc's in that patch and email with it and reply-all to
>   it did not get into mailing list probably due to being considered as spam )
> 
> stream_open.cocci was issuing only warning for pci/switchtec, but after
> 8a29a3bae2a2 ("pci/switchtec: Don't use completion's wait queue") they
> started to use wait_even_* inside read method and, since
> stream_open.cocci considers wait_event_* as blocking the warning became
> error. Previously it was completions there, but I added support for wait
> events only for simplicity.

why is wait_event_interruptible() treated differently compared to
wait_for_completion_interruptible()?
> Kirill

Sebastian
Kirill Smelkov April 15, 2019, 2:55 p.m. UTC | #4
Hi Sebastian,

On Mon, Apr 15, 2019 at 04:38:57PM +0200, Sebastian Andrzej Siewior wrote:
> On 2019-04-13 17:00:59 [+0000], Kirill Smelkov wrote:
> > Hello everyone,
> Hi,
> 
> > On Sat, Apr 13, 2019 at 06:50:57PM +0200, Julia Lawall wrote:
> > > Hello,
> > > 
> > > Kirill will explain about this issue.
> > 
> > pci/switchtec switching to stream_open is already queued to merge
> > window and it was acked by Logan Gunthorpe:
> > 
> >         https://lore.kernel.org/lkml/CAHk-=wgqgN5j1ZWnyVLqqoyU=CCWTYOko3MDyU8L_5e21KvHAg@mail.gmail.com/
> >         https://lab.nexedi.com/kirr/linux/commit/edaeb4101860
> > 
> > ( there are too many Cc's in that patch and email with it and reply-all to
> >   it did not get into mailing list probably due to being considered as spam )
> > 
> > stream_open.cocci was issuing only warning for pci/switchtec, but after
> > 8a29a3bae2a2 ("pci/switchtec: Don't use completion's wait queue") they
> > started to use wait_even_* inside read method and, since
> > stream_open.cocci considers wait_event_* as blocking the warning became
> > error. Previously it was completions there, but I added support for wait
> > events only for simplicity.
> 
> why is wait_event_interruptible() treated differently compared to
> wait_for_completion_interruptible()?

No particular reason. I just taught stream_open.cocci to consider
only "wait_event_*" as blocking:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/coccinelle/api/stream_open.cocci?h=v5.1-rc5#n35

based on original /proc/xen/xenbus deadlock:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/xen/xenbus/xenbus_dev_frontend.c?h=v5.1-rc5#n135
https://git.kernel.org/linus/581d21a2d02a

We can extend "a function that blocks" rule to cover other kernel
primitives.

For the reference: the deadlock scenario is described in

https://git.kernel.org/linus/10dce8af3422

Kirill
Sebastian Andrzej Siewior April 15, 2019, 3:20 p.m. UTC | #5
On 2019-04-15 14:55:02 [+0000], Kirill Smelkov wrote:
> Hi Sebastian,
Hi Kirill,

> On Mon, Apr 15, 2019 at 04:38:57PM +0200, Sebastian Andrzej Siewior wrote:
> > On 2019-04-13 17:00:59 [+0000], Kirill Smelkov wrote:
> > > stream_open.cocci was issuing only warning for pci/switchtec, but after
> > > 8a29a3bae2a2 ("pci/switchtec: Don't use completion's wait queue") they
> > > started to use wait_even_* inside read method and, since
> > > stream_open.cocci considers wait_event_* as blocking the warning became
> > > error. Previously it was completions there, but I added support for wait
> > > events only for simplicity.
> > 
> > why is wait_event_interruptible() treated differently compared to
> > wait_for_completion_interruptible()?
> 
> No particular reason. I just taught stream_open.cocci to consider
> only "wait_event_*" as blocking:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/coccinelle/api/stream_open.cocci?h=v5.1-rc5#n35
> 
> based on original /proc/xen/xenbus deadlock:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/xen/xenbus/xenbus_dev_frontend.c?h=v5.1-rc5#n135
> https://git.kernel.org/linus/581d21a2d02a
> 
> We can extend "a function that blocks" rule to cover other kernel
> primitives.
> 
> For the reference: the deadlock scenario is described in
> 
> https://git.kernel.org/linus/10dce8af3422

As far I understand the problem is when the ->read() callback waits for
the ->write() callback. The locking isn't changed by patch you
mentioned.
So extended might make sense. But then wait_event_* by itself in
->read() isn't a problem as long as its counter part isn't in ->write().
But yes, nice finding.

> Kirill

Sebastian
Kirill Smelkov April 15, 2019, 3:41 p.m. UTC | #6
Hi Sebastian,

On Mon, Apr 15, 2019 at 05:20:22PM +0200, Sebastian Andrzej Siewior wrote:
> On 2019-04-15 14:55:02 [+0000], Kirill Smelkov wrote:
> > Hi Sebastian,
> Hi Kirill,
> 
> > On Mon, Apr 15, 2019 at 04:38:57PM +0200, Sebastian Andrzej Siewior wrote:
> > > On 2019-04-13 17:00:59 [+0000], Kirill Smelkov wrote:
> > > > stream_open.cocci was issuing only warning for pci/switchtec, but after
> > > > 8a29a3bae2a2 ("pci/switchtec: Don't use completion's wait queue") they
> > > > started to use wait_even_* inside read method and, since
> > > > stream_open.cocci considers wait_event_* as blocking the warning became
> > > > error. Previously it was completions there, but I added support for wait
> > > > events only for simplicity.
> > > 
> > > why is wait_event_interruptible() treated differently compared to
> > > wait_for_completion_interruptible()?
> > 
> > No particular reason. I just taught stream_open.cocci to consider
> > only "wait_event_*" as blocking:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/coccinelle/api/stream_open.cocci?h=v5.1-rc5#n35
> > 
> > based on original /proc/xen/xenbus deadlock:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/xen/xenbus/xenbus_dev_frontend.c?h=v5.1-rc5#n135
> > https://git.kernel.org/linus/581d21a2d02a
> > 
> > We can extend "a function that blocks" rule to cover other kernel
> > primitives.
> > 
> > For the reference: the deadlock scenario is described in
> > 
> > https://git.kernel.org/linus/10dce8af3422
> 
> As far I understand the problem is when the ->read() callback waits for
> the ->write() callback. The locking isn't changed by patch you
> mentioned.

Yes, correct. The patch that I mentioned only adds semantic patch which
find places with such problem and can generate a regular patch to change
locking. Here is that place for pci/switchtec:

https://lab.nexedi.com/kirr/linux/commit/edaeb4101860?expand_all_diffs=1#ccc4baef911c8dad164d4ff29a8c0b287abed7c2_393_393

> So extended might make sense. But then wait_event_* by itself in
> ->read() isn't a problem as long as its counter part isn't in ->write().

It is a problem either if its counterpart is in write _or_ if that
wait_event depends on external source and waiting can be for potentially
unbounded time, like e.g. waiting to receive a character from serial
port or network.

But you are right that even with wait_event used, cases are possible that
there is no blocking that depend on external source and it could be just
e.g. spawn kernel thread to do some limited amount of work and wait for
it to complete. I did not taught stream_open.cocci about that because
when something goes wrong with semantic patch and Coccinelle complains,
it is hard to understand what is going on, and because generally it is
better to convert files that do not depend on position, even if there is
no deadlock at all, to stream_open - i.e. don't do any f_pos_lock
locking at all.

> But yes, nice finding.

Thanks,

Kirill
Bjorn Helgaas April 17, 2019, 9:54 p.m. UTC | #7
On Sat, Apr 13, 2019 at 06:50:57PM +0200, Julia Lawall wrote:
> Hello,
> 
> Kirill will explain about this issue.
> 
> julia
> 
> ---------- Forwarded message ----------
> Date: Sat, 13 Apr 2019 11:22:51 +0800
> From: kbuild test robot <lkp@intel.com>
> To: kbuild@01.org
> Cc: Julia Lawall <julia.lawall@lip6.fr>
> Subject: [PATCH] pci/switchtec: fix stream_open.cocci warnings
> 
> CC: kbuild-all@01.org
> TO: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> CC: Kurt Schwemmer <kurt.schwemmer@microsemi.com>
> CC: Logan Gunthorpe <logang@deltatee.com>
> CC: Bjorn Helgaas <helgaas@kernel.org>
> CC: linux-pci@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> 
> From: kbuild test robot <lkp@intel.com>
> 
> drivers/pci/switch/switchtec.c:395:1-17: ERROR: switchtec_fops: .read() can deadlock .write(); change nonseekable_open -> stream_open to fix.
> 
> Generated by: scripts/coccinelle/api/stream_open.cocci
> 
> Fixes: 8a29a3bae2a2 ("pci/switchtec: Don't use completion's wait queue")
> Signed-off-by: kbuild test robot <lkp@intel.com>

Based on Kirill's subsequent email saying this is already queued to
the merge window, I assume I need to do nothing here.

I think a signed-off-by from a robot, i.e., not from a real person, is
meaningless, and I don't think I would personally accept it.  It's
certainly OK to indicate that a patch was auto-generated, but I think
a real person still needs to take responsibility for it.

Documentation/process/submitting-patches.rst says it must contain a
real name (no pseudonyms or anonymous contributions), and I don't
think a robot fits in the spirit of that.

I see that
https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git/commit/?id=8a29a3bae2a2
(mentioned below) does have a good signed-off-by from Sebastian, but
that's not *this* patch, so I don't know what's what.

Bjorn

> ---
> 
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git linux-5.0.y-rt-rebase
> head:   794c294ae4483c240429c25a0d18e272e92c94de
> commit: 8a29a3bae2a2dfb0116cd8791d9700515d6e765e [154/311] pci/switchtec: Don't use completion's wait queue
> :::::: branch date: 7 hours ago
> :::::: commit date: 7 hours ago
> 
> Please take the patch only if it's a positive warning. Thanks!
> 
>  switchtec.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/drivers/pci/switch/switchtec.c
> +++ b/drivers/pci/switch/switchtec.c
> @@ -392,7 +392,7 @@ static int switchtec_dev_open(struct ino
>  		return PTR_ERR(stuser);
> 
>  	filp->private_data = stuser;
> -	nonseekable_open(inode, filp);
> +	stream_open(inode, filp);
> 
>  	dev_dbg(&stdev->dev, "%s: %p\n", __func__, stuser);
>
Julia Lawall April 18, 2019, 5:31 a.m. UTC | #8
On Wed, 17 Apr 2019, Bjorn Helgaas wrote:

> On Sat, Apr 13, 2019 at 06:50:57PM +0200, Julia Lawall wrote:
> > Hello,
> >
> > Kirill will explain about this issue.
> >
> > julia
> >
> > ---------- Forwarded message ----------
> > Date: Sat, 13 Apr 2019 11:22:51 +0800
> > From: kbuild test robot <lkp@intel.com>
> > To: kbuild@01.org
> > Cc: Julia Lawall <julia.lawall@lip6.fr>
> > Subject: [PATCH] pci/switchtec: fix stream_open.cocci warnings
> >
> > CC: kbuild-all@01.org
> > TO: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > CC: Kurt Schwemmer <kurt.schwemmer@microsemi.com>
> > CC: Logan Gunthorpe <logang@deltatee.com>
> > CC: Bjorn Helgaas <helgaas@kernel.org>
> > CC: linux-pci@vger.kernel.org
> > CC: linux-kernel@vger.kernel.org
> >
> > From: kbuild test robot <lkp@intel.com>
> >
> > drivers/pci/switch/switchtec.c:395:1-17: ERROR: switchtec_fops: .read() can deadlock .write(); change nonseekable_open -> stream_open to fix.
> >
> > Generated by: scripts/coccinelle/api/stream_open.cocci
> >
> > Fixes: 8a29a3bae2a2 ("pci/switchtec: Don't use completion's wait queue")
> > Signed-off-by: kbuild test robot <lkp@intel.com>
>
> Based on Kirill's subsequent email saying this is already queued to
> the merge window, I assume I need to do nothing here.
>
> I think a signed-off-by from a robot, i.e., not from a real person, is
> meaningless, and I don't think I would personally accept it.  It's
> certainly OK to indicate that a patch was auto-generated, but I think
> a real person still needs to take responsibility for it.
>
> Documentation/process/submitting-patches.rst says it must contain a
> real name (no pseudonyms or anonymous contributions), and I don't
> think a robot fits in the spirit of that.
>
> I see that
> https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git/commit/?id=8a29a3bae2a2
> (mentioned below) does have a good signed-off-by from Sebastian, but
> that's not *this* patch, so I don't know what's what.

Normally, for these robot generated patches, when I approve them, I put my
own sign off, but under the robot one, since the robot has put a From
line.  In this case, I handed the problem off to Kirill, so I didn't do
that.  I agree that it would be good for Kirill to sign off on it.

julia



>
> Bjorn
>
> > ---
> >
> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git linux-5.0.y-rt-rebase
> > head:   794c294ae4483c240429c25a0d18e272e92c94de
> > commit: 8a29a3bae2a2dfb0116cd8791d9700515d6e765e [154/311] pci/switchtec: Don't use completion's wait queue
> > :::::: branch date: 7 hours ago
> > :::::: commit date: 7 hours ago
> >
> > Please take the patch only if it's a positive warning. Thanks!
> >
> >  switchtec.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > --- a/drivers/pci/switch/switchtec.c
> > +++ b/drivers/pci/switch/switchtec.c
> > @@ -392,7 +392,7 @@ static int switchtec_dev_open(struct ino
> >  		return PTR_ERR(stuser);
> >
> >  	filp->private_data = stuser;
> > -	nonseekable_open(inode, filp);
> > +	stream_open(inode, filp);
> >
> >  	dev_dbg(&stdev->dev, "%s: %p\n", __func__, stuser);
> >
>
Kirill Smelkov April 18, 2019, 10:38 a.m. UTC | #9
On Thu, Apr 18, 2019 at 07:31:02AM +0200, Julia Lawall wrote:
> 
> 
> On Wed, 17 Apr 2019, Bjorn Helgaas wrote:
> 
> > On Sat, Apr 13, 2019 at 06:50:57PM +0200, Julia Lawall wrote:
> > > Hello,
> > >
> > > Kirill will explain about this issue.
> > >
> > > julia
> > >
> > > ---------- Forwarded message ----------
> > > Date: Sat, 13 Apr 2019 11:22:51 +0800
> > > From: kbuild test robot <lkp@intel.com>
> > > To: kbuild@01.org
> > > Cc: Julia Lawall <julia.lawall@lip6.fr>
> > > Subject: [PATCH] pci/switchtec: fix stream_open.cocci warnings
> > >
> > > CC: kbuild-all@01.org
> > > TO: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > > CC: Kurt Schwemmer <kurt.schwemmer@microsemi.com>
> > > CC: Logan Gunthorpe <logang@deltatee.com>
> > > CC: Bjorn Helgaas <helgaas@kernel.org>
> > > CC: linux-pci@vger.kernel.org
> > > CC: linux-kernel@vger.kernel.org
> > >
> > > From: kbuild test robot <lkp@intel.com>
> > >
> > > drivers/pci/switch/switchtec.c:395:1-17: ERROR: switchtec_fops: .read() can deadlock .write(); change nonseekable_open -> stream_open to fix.
> > >
> > > Generated by: scripts/coccinelle/api/stream_open.cocci
> > >
> > > Fixes: 8a29a3bae2a2 ("pci/switchtec: Don't use completion's wait queue")
> > > Signed-off-by: kbuild test robot <lkp@intel.com>
> >
> > Based on Kirill's subsequent email saying this is already queued to
> > the merge window, I assume I need to do nothing here.
> >
> > I think a signed-off-by from a robot, i.e., not from a real person, is
> > meaningless, and I don't think I would personally accept it.  It's
> > certainly OK to indicate that a patch was auto-generated, but I think
> > a real person still needs to take responsibility for it.
> >
> > Documentation/process/submitting-patches.rst says it must contain a
> > real name (no pseudonyms or anonymous contributions), and I don't
> > think a robot fits in the spirit of that.
> >
> > I see that
> > https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git/commit/?id=8a29a3bae2a2
> > (mentioned below) does have a good signed-off-by from Sebastian, but
> > that's not *this* patch, so I don't know what's what.
> 
> Normally, for these robot generated patches, when I approve them, I put my
> own sign off, but under the robot one, since the robot has put a From
> line.  In this case, I handed the problem off to Kirill, so I didn't do
> that.  I agree that it would be good for Kirill to sign off on it.

Just for the reference: I have put my signature on the mass converstion
patch as well as ack's that were received:

https://lab.nexedi.com/kirr/linux/commit/edaeb4101860

I plan to refresh that patch and post it to mainline when merge-window opens.

Appologize if I missed something. I'm not a kernel developer and don't
have much know-how in this land.

Kirill


> > > ---
> > >
> > > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git linux-5.0.y-rt-rebase
> > > head:   794c294ae4483c240429c25a0d18e272e92c94de
> > > commit: 8a29a3bae2a2dfb0116cd8791d9700515d6e765e [154/311] pci/switchtec: Don't use completion's wait queue
> > > :::::: branch date: 7 hours ago
> > > :::::: commit date: 7 hours ago
> > >
> > > Please take the patch only if it's a positive warning. Thanks!
> > >
> > >  switchtec.c |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > --- a/drivers/pci/switch/switchtec.c
> > > +++ b/drivers/pci/switch/switchtec.c
> > > @@ -392,7 +392,7 @@ static int switchtec_dev_open(struct ino
> > >  		return PTR_ERR(stuser);
> > >
> > >  	filp->private_data = stuser;
> > > -	nonseekable_open(inode, filp);
> > > +	stream_open(inode, filp);
> > >
> > >  	dev_dbg(&stdev->dev, "%s: %p\n", __func__, stuser);
Bjorn Helgaas April 18, 2019, 12:37 p.m. UTC | #10
On Thu, Apr 18, 2019 at 10:38:02AM +0000, Kirill Smelkov wrote:
> On Thu, Apr 18, 2019 at 07:31:02AM +0200, Julia Lawall wrote:
> > On Wed, 17 Apr 2019, Bjorn Helgaas wrote:
> > > On Sat, Apr 13, 2019 at 06:50:57PM +0200, Julia Lawall wrote:
> > > > Hello,
> > > >
> > > > Kirill will explain about this issue.
> > > >
> > > > julia
> > > >
> > > > ---------- Forwarded message ----------
> > > > Date: Sat, 13 Apr 2019 11:22:51 +0800
> > > > From: kbuild test robot <lkp@intel.com>
> > > > To: kbuild@01.org
> > > > Cc: Julia Lawall <julia.lawall@lip6.fr>
> > > > Subject: [PATCH] pci/switchtec: fix stream_open.cocci warnings
> > > >
> > > > CC: kbuild-all@01.org
> > > > TO: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > > > CC: Kurt Schwemmer <kurt.schwemmer@microsemi.com>
> > > > CC: Logan Gunthorpe <logang@deltatee.com>
> > > > CC: Bjorn Helgaas <helgaas@kernel.org>
> > > > CC: linux-pci@vger.kernel.org
> > > > CC: linux-kernel@vger.kernel.org
> > > >
> > > > From: kbuild test robot <lkp@intel.com>
> > > >
> > > > drivers/pci/switch/switchtec.c:395:1-17: ERROR: switchtec_fops: .read() can deadlock .write(); change nonseekable_open -> stream_open to fix.
> > > >
> > > > Generated by: scripts/coccinelle/api/stream_open.cocci
> > > >
> > > > Fixes: 8a29a3bae2a2 ("pci/switchtec: Don't use completion's wait queue")
> > > > Signed-off-by: kbuild test robot <lkp@intel.com>
> > >
> > > Based on Kirill's subsequent email saying this is already queued to
> > > the merge window, I assume I need to do nothing here.
> > >
> > > I think a signed-off-by from a robot, i.e., not from a real person, is
> > > meaningless, and I don't think I would personally accept it.  It's
> > > certainly OK to indicate that a patch was auto-generated, but I think
> > > a real person still needs to take responsibility for it.
> > >
> > > Documentation/process/submitting-patches.rst says it must contain a
> > > real name (no pseudonyms or anonymous contributions), and I don't
> > > think a robot fits in the spirit of that.
> > >
> > > I see that
> > > https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git/commit/?id=8a29a3bae2a2
> > > (mentioned below) does have a good signed-off-by from Sebastian, but
> > > that's not *this* patch, so I don't know what's what.
> > 
> > Normally, for these robot generated patches, when I approve them, I put my
> > own sign off, but under the robot one, since the robot has put a From
> > line.  In this case, I handed the problem off to Kirill, so I didn't do
> > that.  I agree that it would be good for Kirill to sign off on it.
> 
> Just for the reference: I have put my signature on the mass converstion
> patch as well as ack's that were received:
> 
> https://lab.nexedi.com/kirr/linux/commit/edaeb4101860

Looks good, thanks.  Feel free to add my

  Acked-by: Bjorn Helgaas <bhelgaas@google.com> [drivers/pci/switch/switchtec]

to the https://lab.nexedi.com/kirr/linux/commit/edaeb4101860 patch.

It looks like maybe the commit log could use

  s/and the reset were/and the rest were/

It also mentions "the previous patch" a couple times, which may lose
some of its meaning depending on how things get merged into git.  If
that previous patch has already been merged, a SHA1 reference would be
more specific.

I would personally split that into two patches: one to avoid the
potential deadlocks and a second to do the "safe to change to
stream_open" changes.  It seems like the first is more serious and
urgent while the second is more of a cleanup.  Then you could
streamline the commit logs by including a single diagnostic and
omitting the entire list of files.

But that's all bike-shedding and I'm totally fine with this as-is.

Bjorn
Kirill Smelkov April 18, 2019, 2:42 p.m. UTC | #11
On Thu, Apr 18, 2019 at 07:37:30AM -0500, Bjorn Helgaas wrote:
> On Thu, Apr 18, 2019 at 10:38:02AM +0000, Kirill Smelkov wrote:
> > On Thu, Apr 18, 2019 at 07:31:02AM +0200, Julia Lawall wrote:
> > > On Wed, 17 Apr 2019, Bjorn Helgaas wrote:
> > > > On Sat, Apr 13, 2019 at 06:50:57PM +0200, Julia Lawall wrote:
> > > > > Hello,
> > > > >
> > > > > Kirill will explain about this issue.
> > > > >
> > > > > julia
> > > > >
> > > > > ---------- Forwarded message ----------
> > > > > Date: Sat, 13 Apr 2019 11:22:51 +0800
> > > > > From: kbuild test robot <lkp@intel.com>
> > > > > To: kbuild@01.org
> > > > > Cc: Julia Lawall <julia.lawall@lip6.fr>
> > > > > Subject: [PATCH] pci/switchtec: fix stream_open.cocci warnings
> > > > >
> > > > > CC: kbuild-all@01.org
> > > > > TO: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > > > > CC: Kurt Schwemmer <kurt.schwemmer@microsemi.com>
> > > > > CC: Logan Gunthorpe <logang@deltatee.com>
> > > > > CC: Bjorn Helgaas <helgaas@kernel.org>
> > > > > CC: linux-pci@vger.kernel.org
> > > > > CC: linux-kernel@vger.kernel.org
> > > > >
> > > > > From: kbuild test robot <lkp@intel.com>
> > > > >
> > > > > drivers/pci/switch/switchtec.c:395:1-17: ERROR: switchtec_fops: .read() can deadlock .write(); change nonseekable_open -> stream_open to fix.
> > > > >
> > > > > Generated by: scripts/coccinelle/api/stream_open.cocci
> > > > >
> > > > > Fixes: 8a29a3bae2a2 ("pci/switchtec: Don't use completion's wait queue")
> > > > > Signed-off-by: kbuild test robot <lkp@intel.com>
> > > >
> > > > Based on Kirill's subsequent email saying this is already queued to
> > > > the merge window, I assume I need to do nothing here.
> > > >
> > > > I think a signed-off-by from a robot, i.e., not from a real person, is
> > > > meaningless, and I don't think I would personally accept it.  It's
> > > > certainly OK to indicate that a patch was auto-generated, but I think
> > > > a real person still needs to take responsibility for it.
> > > >
> > > > Documentation/process/submitting-patches.rst says it must contain a
> > > > real name (no pseudonyms or anonymous contributions), and I don't
> > > > think a robot fits in the spirit of that.
> > > >
> > > > I see that
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git/commit/?id=8a29a3bae2a2
> > > > (mentioned below) does have a good signed-off-by from Sebastian, but
> > > > that's not *this* patch, so I don't know what's what.
> > > 
> > > Normally, for these robot generated patches, when I approve them, I put my
> > > own sign off, but under the robot one, since the robot has put a From
> > > line.  In this case, I handed the problem off to Kirill, so I didn't do
> > > that.  I agree that it would be good for Kirill to sign off on it.
> > 
> > Just for the reference: I have put my signature on the mass converstion
> > patch as well as ack's that were received:
> > 
> > https://lab.nexedi.com/kirr/linux/commit/edaeb4101860
> 
> Looks good, thanks.  Feel free to add my
> 
>   Acked-by: Bjorn Helgaas <bhelgaas@google.com> [drivers/pci/switch/switchtec]
> 
> to the https://lab.nexedi.com/kirr/linux/commit/edaeb4101860 patch.
> 
> It looks like maybe the commit log could use
> 
>   s/and the reset were/and the rest were/

Bjorn, thanks for feedback. I've added your ack and fixed that typo.

> It also mentions "the previous patch" a couple times, which may lose
> some of its meaning depending on how things get merged into git.  If
> that previous patch has already been merged, a SHA1 reference would be
> more specific.

Good point. Initially those patches were coming together, but the
first one landed to master while the conversion is only scheduled to be
done. I've changed this reference to 10dce8af3422 ("fs: stream_open -
opener for stream-like files so that read and write can run
simultaneously without deadlock").

> I would personally split that into two patches: one to avoid the
> potential deadlocks and a second to do the "safe to change to
> stream_open" changes.  It seems like the first is more serious and
> urgent while the second is more of a cleanup.  Then you could
> streamline the commit logs by including a single diagnostic and
> omitting the entire list of files.

I was contemplating how to split this too. And one of the way was to go
with separate patch for every subsystem. However I still hope to do the
mass conversion all at one conversion, because otherwise it would be
many patches and it will take my time to propagate them all / ping
maintainers etc.  About splitting deadlock / just safe:
stream_open.cocci does not have complete coverage for detecting whether
a .read() blocks, and as pci/switchtec case shows there are indeed other
cases that might be deadlocking but are not currently detected as such.
I would thus prefer not to split the conversion.

I've added the following note to the patch:

    and the rest were just safe to convert to stream_open because their read and
    write do not use ppos at all and corresponding file_operations do not
    have methods that assume @offset file access(*):

    <long list>

    ...

    (*) This second group also contains cases with read/write deadlocks that
    stream_open.cocci don't yet detect, but which are still valid to convert to
    stream_open since ppos is not used. For example drivers/pci/switch/switchtec.c
    calls wait_for_completion_interruptible() in its .read, but stream_open.cocci
    currently detects only "wait_event*" as blocking.

hope it is ok.

> But that's all bike-shedding and I'm totally fine with this as-is.

Thanks. Your input was useful. The updated patch is here:

https://lab.nexedi.com/kirr/linux/commit/a34a8a9cb2d7

as well as related patches to tighten stream_open semantic and
corresponding FUSE bits:

https://lab.nexedi.com/kirr/linux/commit/47ee8df337a9
https://lab.nexedi.com/kirr/linux/commit/1b4636172835

Kirill
diff mbox series

Patch

--- a/drivers/pci/switch/switchtec.c
+++ b/drivers/pci/switch/switchtec.c
@@ -392,7 +392,7 @@  static int switchtec_dev_open(struct ino
 		return PTR_ERR(stuser);

 	filp->private_data = stuser;
-	nonseekable_open(inode, filp);
+	stream_open(inode, filp);

 	dev_dbg(&stdev->dev, "%s: %p\n", __func__, stuser);