On Wed, Dec 11, 2024 at 09:59:30AM +0100, Thomas Richard wrote:

On 12/10/24 16:42, Bjorn Helgaas wrote:

quoted

On Mon, Dec 09, 2024 at 12:23:21PM +0100, Thomas Richard wrote:

quoted

The reset_gpio is optional, so in j721e_pcie_suspend_noirq() check if it is
not NULL before to use it.

If you have occasion to post a v2, update subject to:

  PCI: j721e: Check reset_gpio for NULL before using it

s/before to use it/before using it/

Did you trip over a NULL pointer dereference here?  Or maybe found via
inspection?

By inspection

quoted

It looks like gpiod_set_value_cansleep(desc) *should* be a no-op if
desc is NULL, based on this comment [1]:

 * This descriptor validation needs to be inserted verbatim into each
 * function taking a descriptor, so we need to use a preprocessor
 * macro to avoid endless duplication. If the desc is NULL it is an
 * optional GPIO and calls should just bail out.

and the fact that the VALIDATE_DESC_VOID() macro looks like it would
return early in that case.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?id=v6.12#n2316

Oh yes you're right.
In fact, the if statement in probe() and resume_noirq() is for msleep(),
not really for gpiod_set_value_cansleep().

So this patch is useless.

Yes. Almost all of the GPIO APIs accepting desc (except few) use VALIDATE_DESC()
to check for NULL descriptor. So explicit check is not needed.

- Mani

-- 
மணிவண்ணன் சதாசிவம்