Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210525125925.112306-1-lambert.q.wang@gmail.com> <20210525132035.GA66609@rocinante.localdomain>
In-Reply-To: <20210525132035.GA66609@rocinante.localdomain>
From:   Lambert Wang <lambert.q.wang@gmail.com>
Date:   Wed, 26 May 2021 14:12:38 +0800
Message-ID: <CAATamay8WTiJnB=5OLYdFTqVUcRF9LarN6_1Eej3QUgFzWRnkA@mail.gmail.com>
Subject: Re: [PATCH] pci: add pci_dev_is_alive API
To:     =?UTF-8?Q?Krzysztof_Wilczy=C5=84ski?= <kw@linux.com>,
        Bjorn Helgaas <bhelgaas@google.com>
Cc:     linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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=C5=84ski <kw@linux.com> wr=
ote:
>
> 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#functi=
on-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 aliv=
e
> > + * 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 =3D=3D pdev->vendor;
> > +}
> > +EXPORT_SYMBOL(pci_dev_is_alive);
>
> Why not use the EXPORT_SYMBOL_GPL()?
>
>         Krzysztof