Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230809232851.1004023-1-alistair.francis@wdc.com> <20230810073457.GA26246@wunner.de>
In-Reply-To: <20230810073457.GA26246@wunner.de>
From:   Alistair Francis <alistair23@gmail.com>
Date:   Thu, 10 Aug 2023 11:34:11 -0400
Message-ID: <CAKmqyKPm_BFnNxVLXCO_PVRDJaVb+XOj=kEEzXd+MgkwDiZhXA@mail.gmail.com>
Subject: Re: [PATCH v3] PCI/DOE: Expose the DOE protocols via sysfs
To:     Lukas Wunner <lukas@wunner.de>
Cc:     bhelgaas@google.com, linux-pci@vger.kernel.org,
        Jonathan.Cameron@huawei.com, alex.williamson@redhat.com,
        christian.koenig@amd.com, kch@nvidia.com,
        gregkh@linuxfoundation.org, logang@deltatee.com,
        linux-kernel@vger.kernel.org,
        Alistair Francis <alistair.francis@wdc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Aug 10, 2023 at 3:34=E2=80=AFAM Lukas Wunner <lukas@wunner.de> wrot=
e:
>
> On Wed, Aug 09, 2023 at 07:28:51PM -0400, Alistair Francis wrote:
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -1226,6 +1227,12 @@ static int pci_create_resource_files(struct pci_=
dev *pdev)
> >       int i;
> >       int retval;
> >
> > +#ifdef CONFIG_PCI_DOE
> > +     retval =3D doe_sysfs_init(pdev);
> > +     if (retval)
> > +             return retval;
> > +#endif
> > +
>
> The preferred way to expose PCI sysfs attributes nowadays is to add them
> to pci_dev_attr_groups[] and use the ->is_visible callback to check
> whether they're applicable to a particular pci_dev.  The alternative
> via pci_create_resource_files() has race conditions which I think
> still haven't been fixed. Bjorn recommended the ->is_visible approach
> in response to the most recent attempt to fix the race:
>
> https://lore.kernel.org/linux-pci/20230427161458.GA249886@bhelgaas/

The is_visible doen't seem to work in this case.

AFAIK is_visible only applies to the attributes under the group. Which
means that every PCIe device will see a `doe_protos` directory, no
matter if DOE is supported.

On top of that is_visible() is only called
(fs/sysfs/group.c:create_files()) if there are sub attrs, which we
don't have. We will possibly end up with some attributes, but all of
them are dynamic. So we still end up needing to call doe_sysfs_init()
anyway.

Alistair

>
> To avoid (most of) the #ifdefs, you may want to consider adding a
> doe-sysfs.c file like I've done for cma in this commit:
>
> https://github.com/l1k/linux/commit/b057e2fb0ee0
>
> Thanks,
>
> Lukas