Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
MIME-Version: 1.0
References: <20231013034158.2745127-1-alistair.francis@wdc.com>
 <20231013034158.2745127-2-alistair.francis@wdc.com> <20231017083421.GA23168@wunner.de>
In-Reply-To: <20231017083421.GA23168@wunner.de>
From:   Alistair Francis <alistair23@gmail.com>
Date:   Fri, 3 Nov 2023 11:27:02 +1000
Message-ID: <CAKmqyKM5AtH=b_Twt2FY6+dwxgjDwsk6K1mJ88x0i9T9EAbK5w@mail.gmail.com>
Subject: Re: [PATCH v9 2/3] PCI/DOE: Expose the DOE features 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, chaitanyak@nvidia.com,
        rdunlap@infradead.org, Alistair Francis <alistair.francis@wdc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Oct 17, 2023 at 6:34=E2=80=AFPM Lukas Wunner <lukas@wunner.de> wrot=
e:
>
> On Fri, Oct 13, 2023 at 01:41:57PM +1000, Alistair Francis wrote:
> > +#ifdef CONFIG_SYSFS
> > +static umode_t pci_doe_sysfs_attr_is_visible(struct kobject *kobj,
> > +                                          struct attribute *a, int n)
> > +{
> > +     struct pci_dev *pdev =3D to_pci_dev(kobj_to_dev(kobj));
> > +     struct pci_doe_mb *doe_mb;
> > +     unsigned long index, j;
> > +     void *entry;
> > +
> > +     xa_for_each(&pdev->doe_mbs, index, doe_mb) {
> > +             xa_for_each(&doe_mb->feats, j, entry)
> > +                     return a->mode;
> > +     }
> > +
> > +     return 0;
> > +}
>
> Out of curiosity:  Does this method prevent creation of a
> "doe_features" directory for devices which don't have any
> DOE mailboxes?

It does once this patch (or something similar) is applied:

https://lkml.org/lkml/2022/8/24/607

GKH and I are working on getting a patch like that working and merged

Alistair

>
> (If it does, a code comment explaining that might be helpful.)
>
>
> > +const struct attribute_group pci_dev_doe_feature_group =3D {
> > +     .name   =3D "doe_features",
> > +     .attrs  =3D pci_dev_doe_feature_attrs,
> > +     .is_visible =3D pci_doe_sysfs_attr_is_visible,
> > +};
>
> Nit:  Wondering why the "=3D" is aligned for .name and .attrs
> but not for .is_visible?
>
>
> > +static void pci_doe_sysfs_feature_remove(struct pci_dev *pdev,
> > +                                      struct pci_doe_mb *doe_mb)
> > +{
> > +     struct device *dev =3D &pdev->dev;
> > +     struct device_attribute *attrs =3D doe_mb->sysfs_attrs;
> > +     unsigned long i;
> > +     void *entry;
>
> Nit:  Inverse Christmas tree?
>
>
> > +static int pci_doe_sysfs_feature_populate(struct pci_dev *pdev,
> > +                                       struct pci_doe_mb *doe_mb)
> > +{
> > +     struct device *dev =3D &pdev->dev;
> > +     struct device_attribute *attrs;
> > +     unsigned long num_features =3D 0;
> > +     unsigned long vid, type;
> > +     unsigned long i;
> > +     void *entry;
> > +     int ret;
> > +
> > +     xa_for_each(&doe_mb->feats, i, entry)
> > +             num_features++;
> > +
> > +     attrs =3D kcalloc(num_features, sizeof(*attrs), GFP_KERNEL);
> > +     if (!attrs)
> > +             return -ENOMEM;
> > +
> > +     doe_mb->sysfs_attrs =3D attrs;
> > +     xa_for_each(&doe_mb->feats, i, entry) {
> > +             sysfs_attr_init(&attrs[i].attr);
> > +             vid =3D xa_to_value(entry) >> 8;
> > +             type =3D xa_to_value(entry) & 0xFF;
> > +             attrs[i].attr.name =3D kasprintf(GFP_KERNEL,
> > +                                            "0x%04lX:%02lX", vid, type=
);
>
> Nit:  Capital X conversion specifier will result in upper case hex
> characters in filename and contents, whereas existing sysfs attributes
> such as "vendor", "device" contain lower case hex characters.
>
> Might want to consider lower case x for consistency.
>
>
> > +void pci_doe_sysfs_teardown(struct pci_dev *pdev)
> > +{
> > +     struct pci_doe_mb *doe_mb;
> > +     unsigned long index;
> > +
> > +     xa_for_each(&pdev->doe_mbs, index, doe_mb) {
> > +             pci_doe_sysfs_feature_remove(pdev, doe_mb);
> > +     }
>
> Nit:  Curly braces not necessary.
>
>
> > @@ -1153,6 +1154,10 @@ static void pci_remove_resource_files(struct pci=
_dev *pdev)
> >  {
> >       int i;
> >
> > +     if (IS_ENABLED(CONFIG_PCI_DOE)) {
> > +             pci_doe_sysfs_teardown(pdev);
> > +     }
>
> Nit:  Curly braces not necessary.
>
>
> > @@ -1230,6 +1235,12 @@ static int pci_create_resource_files(struct pci_=
dev *pdev)
> >       int i;
> >       int retval;
> >
> > +     if (IS_ENABLED(CONFIG_PCI_DOE)) {
> > +             retval =3D pci_doe_sysfs_init(pdev);
> > +             if (retval)
> > +                     return retval;
> > +     }
> > +
> >       /* Expose the PCI resources from this device as files */
> >       for (i =3D 0; i < PCI_STD_NUM_BARS; i++) {
>
> I think this needs to be added to pci_create_sysfs_dev_files() instead.
>
> pci_create_resource_files() only deals with creation of resource files,
> as the name implies, which is unrelated to DOE features.
>
> Worse, pci_create_resource_files() is also called from
> pci_dev_resource_resize_attr(), i.e. every time user space
> writes to the "resource##n##_resize" attributes.
>
> Similarly, the call to pci_doe_sysfs_teardown() belongs in
> pci_remove_sysfs_dev_files().
>
> Thanks,
>
> Lukas