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@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/

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