linux-kernel - Re: [PATCH] pci: add pci_dev_is

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAATamay8WTiJnB=5OLYdFTqVUcRF9LarN6_1Eej3QUgFzWRnkA@mail.gmail.com>
Date:   Wed, 26 May 2021 14:12:38 +0800
From:   Lambert Wang <lambert.q.wang@...il.com>
To:     Krzysztof Wilczyński <kw@...ux.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>
Cc:     linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] pci: add pci_dev_is_alive API

Hi  Krzysztof and Bjorn,

Thanks for your comments and your time.Your questions are answered inline.

I have checked and tested *pci_device_is_present* as pointed out by Bjorn.
I think that API could satisfy my needs.

So this patch request could be dropped. Sorry for the inconvenience.

On Tue, May 25, 2021 at 9:20 PM Krzysztof Wilczyński <kw@...ux.com> wrote:
>
> Hi Lambert,
>
> Thank you for sending the patch over!
>
> To match the style of other patches you'd need to capitalise "PCI" in
> the subject, see the following for some examples:
>
>  $ git log --oneline drivers/pci/pci.c
>
> Also, it might be worth mentioning in the subject that this is a new API
> that will be added.
>

I will be careful on the styles of patch title and description in my
future patches.

> > Device drivers use this API to proactively check if the device
> > is alive or not. It is helpful for some PCI devices to detect
> > surprise removal and do recovery when Hotplug function is disabled.
> >
> > Note: Device in power states larger than D0 is also treated not alive
> > by this function.
> [...]
>
> Question to you: do you have any particular users of this new API in
> mind?  Or is this solving some problem you've seen and/or reported via
> the kernel Bugzilla?
>

The user is our new PCI driver under development for WWAN devices .
Surprise removal could happen under multiple circumstances.
e.g. Exception, Link Failure, etc.

We wanted this API to detect surprise removal or check device recovery
when AER and Hotplug are disabled.

I thought the API could be commonly used for many similar devices.

> > +/**
> > + * pci_dev_is_alive - check the pci device is alive or not
> > + * @pdev: the PCI device
>
> That would be "PCI" in the function brief above.  Also, try to be
> consistent and capitalise everything plus add missing periods, see the
> following for an example on how to write kernel-doc:
>
>   https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation
>
> > + * Device drivers use this API to proactively check if the device
> > + * is alive or not. It is helpful for some PCI devices to detect
> > + * surprise removal and do recovery when Hotplug function is disabled.
>
> As per my question above - what users of this new API do you have in
> mind?  Are they any other patches pending adding users of this API?
>
> > + * Note: Device in power state larger than D0 is also treated not alive
> > + * by this function.
> > + *
> > + * Returns true if the device is alive.
> > + */
> > +bool pci_dev_is_alive(struct pci_dev *pdev)
> > +{
> > +     u16 vendor;
> > +
> > +     pci_read_config_word(pdev, PCI_VENDOR_ID, &vendor);
> > +
> > +     return vendor == pdev->vendor;
> > +}
> > +EXPORT_SYMBOL(pci_dev_is_alive);
>
> Why not use the EXPORT_SYMBOL_GPL()?
>
>         Krzysztof