On Wed, Dec 11, 2024 at 09:59:30AM +0100, Thomas Richard wrote: > On 12/10/24 16:42, Bjorn Helgaas wrote: > > On Mon, Dec 09, 2024 at 12:23:21PM +0100, Thomas Richard wrote: > >> 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 > > > > > 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 -- மணிவண்ணன் சதாசிவம்
