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