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@xxxxxxxxx>
> > > > To: kbuild@xxxxxx
> > > > Cc: Julia Lawall <julia.lawall@xxxxxxx>
> > > > Subject: [PATCH] pci/switchtec: fix stream_open.cocci warnings
> > > >
> > > > CC: kbuild-all@xxxxxx
> > > > TO: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> > > > CC: Kurt Schwemmer <kurt.schwemmer@xxxxxxxxxxxxx>
> > > > CC: Logan Gunthorpe <logang@xxxxxxxxxxxx>
> > > > CC: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> > > > CC: linux-pci@xxxxxxxxxxxxxxx
> > > > CC: linux-kernel@xxxxxxxxxxxxxxx
> > > >
> > > > From: kbuild test robot <lkp@xxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx> [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