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

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

[Julia Lawall]

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(-)

--- 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);

Re: [PATCH] pci/switchtec: fix stream_open.cocci warnings (fwd)

[Logan Gunthorpe]

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);
> 

Re: [PATCH] pci/switchtec: fix stream_open.cocci warnings (fwd)

[Kirill Smelkov]

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);

Re: [PATCH] pci/switchtec: fix stream_open.cocci warnings (fwd)

[Sebastian Andrzej Siewior]

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

Re: [PATCH] pci/switchtec: fix stream_open.cocci warnings (fwd)

[Kirill Smelkov]

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

Re: [PATCH] pci/switchtec: fix stream_open.cocci warnings (fwd)

[Sebastian Andrzej Siewior]

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

Re: [PATCH] pci/switchtec: fix stream_open.cocci warnings (fwd)

[Kirill Smelkov]

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

Re: [PATCH] pci/switchtec: fix stream_open.cocci warnings (fwd)

[Bjorn Helgaas]

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);
> 

Re: [PATCH] pci/switchtec: fix stream_open.cocci warnings (fwd)

[Julia Lawall]

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);
> >
>

Re: [PATCH] pci/switchtec: fix stream_open.cocci warnings (fwd)

[Kirill Smelkov]

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);

Re: [PATCH] pci/switchtec: fix stream_open.cocci warnings (fwd)

[Bjorn Helgaas]

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

Re: [PATCH] pci/switchtec: fix stream_open.cocci warnings (fwd)

[Kirill Smelkov]

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

Re: [PATCH] pci/switchtec: fix stream_open.cocci warnings (fwd)

[Kirill Smelkov]

Hi Julia, everyone.

On Wed, Jun 19, 2019 at 12:28:47PM +0200, Julia Lawall wrote:
> Hi,
> 
> Can you forward this patch to the people below if you think it is
> appropriate?

Yes, this patch is appropriate. It was actually part of
git.kernel.org/linus/c5bf68fe0c86 . It should be safe, (and desirable as
it removes a chance for deadlock) to apply it. 

Sebastian, Kurt, Logan, Bjorn, please consider picking it up.

> Could I tell the kbuild people to add you to the CC list for
> this semantic patch?

Yes, sure. Please feel free to add me to CC list for stream_open.cocci
related patches.

Kirill


> thanks,
> julia
> 
> ---------- Forwarded message ----------
> Date: Wed, 19 Jun 2019 18:23:18 +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: a3a1e895d4fa ("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:   31cc76d5590f5e60c2f26f029e40bc7d0441d93f
> commit: a3a1e895d4fa0508e11ac9107ace883a5b2a4d3b [171/305] pci/switchtec: Don't use completion's wait queue
> :::::: branch date: 6 days ago
> :::::: commit date: 6 days 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);
>

Re: [PATCH] pci/switchtec: fix stream_open.cocci warnings (fwd)

[Logan Gunthorpe]

On 2019-06-19 10:27 a.m., Kirill Smelkov wrote:
> Hi Julia, everyone.
> 
> On Wed, Jun 19, 2019 at 12:28:47PM +0200, Julia Lawall wrote:
>> Hi,
>>
>> Can you forward this patch to the people below if you think it is
>> appropriate?

>> 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: a3a1e895d4fa ("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

This is for the RT tree? The patch in the fixes tag isn't in upstream
and I don't understand how this is related to that patch at all. It just
looks like the RT tree hasn't picked up the patch which made this change
in upstream.

I feel like I've seen the change in this patch a bunch of times already
and it appears to be correct in rc5 at least...

Logan


>> head:   31cc76d5590f5e60c2f26f029e40bc7d0441d93f
>> commit: a3a1e895d4fa0508e11ac9107ace883a5b2a4d3b [171/305] pci/switchtec: Don't use completion's wait queue
>> :::::: branch date: 6 days ago
>> :::::: commit date: 6 days 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);
>>

Re: [PATCH] pci/switchtec: fix stream_open.cocci warnings (fwd)

[Bjorn Helgaas]

On Wed, Jun 19, 2019 at 04:27:52PM +0000, Kirill Smelkov wrote:
> Hi Julia, everyone.
> 
> On Wed, Jun 19, 2019 at 12:28:47PM +0200, Julia Lawall wrote:
> > Hi,
> > 
> > Can you forward this patch to the people below if you think it is
> > appropriate?
> 
> Yes, this patch is appropriate. It was actually part of
> git.kernel.org/linus/c5bf68fe0c86 . It should be safe, (and desirable as
> it removes a chance for deadlock) to apply it. 
> 
> Sebastian, Kurt, Logan, Bjorn, please consider picking it up.

I don't get it.  This appeared in v5.2-rc1 as c5bf68fe0c86 ("*: convert
stream-like files from nonseekable_open -> stream_open"), so it looks like
this is already done.  What would you like me to do with it?

> > Could I tell the kbuild people to add you to the CC list for
> > this semantic patch?
> 
> Yes, sure. Please feel free to add me to CC list for stream_open.cocci
> related patches.
> 
> Kirill
> 
> 
> > thanks,
> > julia
> > 
> > ---------- Forwarded message ----------
> > Date: Wed, 19 Jun 2019 18:23:18 +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: a3a1e895d4fa ("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:   31cc76d5590f5e60c2f26f029e40bc7d0441d93f
> > commit: a3a1e895d4fa0508e11ac9107ace883a5b2a4d3b [171/305] pci/switchtec: Don't use completion's wait queue
> > :::::: branch date: 6 days ago
> > :::::: commit date: 6 days 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);
> >

Re: [PATCH] pci/switchtec: fix stream_open.cocci warnings (fwd)

[Julia Lawall]

On Wed, 19 Jun 2019, Bjorn Helgaas wrote:

> On Wed, Jun 19, 2019 at 04:27:52PM +0000, Kirill Smelkov wrote:
> > Hi Julia, everyone.
> >
> > On Wed, Jun 19, 2019 at 12:28:47PM +0200, Julia Lawall wrote:
> > > Hi,
> > >
> > > Can you forward this patch to the people below if you think it is
> > > appropriate?
> >
> > Yes, this patch is appropriate. It was actually part of
> > git.kernel.org/linus/c5bf68fe0c86 . It should be safe, (and desirable as
> > it removes a chance for deadlock) to apply it.
> >
> > Sebastian, Kurt, Logan, Bjorn, please consider picking it up.
>
> I don't get it.  This appeared in v5.2-rc1 as c5bf68fe0c86 ("*: convert
> stream-like files from nonseekable_open -> stream_open"), so it looks like
> this is already done.  What would you like me to do with it?

Somehow 0-day got a hold of it...  If there is nothing to do, just ignore
it.

thanks,
julia

>
> > > Could I tell the kbuild people to add you to the CC list for
> > > this semantic patch?
> >
> > Yes, sure. Please feel free to add me to CC list for stream_open.cocci
> > related patches.
> >
> > Kirill
> >
> >
> > > thanks,
> > > julia
> > >
> > > ---------- Forwarded message ----------
> > > Date: Wed, 19 Jun 2019 18:23:18 +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: a3a1e895d4fa ("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:   31cc76d5590f5e60c2f26f029e40bc7d0441d93f
> > > commit: a3a1e895d4fa0508e11ac9107ace883a5b2a4d3b [171/305] pci/switchtec: Don't use completion's wait queue
> > > :::::: branch date: 6 days ago
> > > :::::: commit date: 6 days 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);
> > >
>

Re: [PATCH] pci/switchtec: fix stream_open.cocci warnings (fwd)

[kirr]

Bjorn Helgaas писал 2019-06-19 23:19:
> On Wed, Jun 19, 2019 at 04:27:52PM +0000, Kirill Smelkov wrote:
>> Hi Julia, everyone.
>>
>> On Wed, Jun 19, 2019 at 12:28:47PM +0200, Julia Lawall wrote:
>> > Hi,
>> >
>> > Can you forward this patch to the people below if you think it is
>> > appropriate?
>>
>> Yes, this patch is appropriate. It was actually part of
>> git.kernel.org/linus/c5bf68fe0c86 . It should be safe, (and desirable
>> as
>> it removes a chance for deadlock) to apply it.
>>
>> Sebastian, Kurt, Logan, Bjorn, please consider picking it up.
>
> I don't get it.  This appeared in v5.2-rc1 as c5bf68fe0c86 ("*: convert
> stream-like files from nonseekable_open -> stream_open"), so it looks
> like
> this is already done.  What would you like me to do with it?

I meant just that it was ok to pick this change into 5.0-RT tree as
kbuild robot was suggesting. Sorry for not being clear.

Kirill

>> > Could I tell the kbuild people to add you to the CC list for
>> > this semantic patch?
>>
>> Yes, sure. Please feel free to add me to CC list for stream_open.cocci
>> related patches.
>>
>> Kirill
>>
>>
>> > thanks,
>> > julia
>> >
>> > ---------- Forwarded message ----------
>> > Date: Wed, 19 Jun 2019 18:23:18 +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: a3a1e895d4fa ("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:   31cc76d5590f5e60c2f26f029e40bc7d0441d93f
>> > commit: a3a1e895d4fa0508e11ac9107ace883a5b2a4d3b [171/305] pci/switchtec: Don't use completion's wait queue
>> > :::::: branch date: 6 days ago
>> > :::::: commit date: 6 days 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);
>> >

Re: [PATCH] pci/switchtec: fix stream_open.cocci warnings (fwd)

[Sebastian Andrzej Siewior]

On 2019-06-20 07:01:43 [+0000], kirr@nexedi.com wrote:
> I meant just that it was ok to pick this change into 5.0-RT tree as
> kbuild robot was suggesting. Sorry for not being clear.

Okay, I was under the impression that this affect non-RT as well. But if
this is RT-only then let me pick it up…

> Kirill

Sebastian