| Message ID | 20241209112321.65320-1-thomas.richard@bootlin.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: j721e: In j721e_pcie_suspend_noirq() check reset_gpio before to use it | expand |
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. > > Fixes: c538d40f365b ("PCI: j721e: Add suspend and resume support") > Signed-off-by: Thomas Richard <thomas.richard@bootlin.com> Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com> > --- > drivers/pci/controller/cadence/pci-j721e.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c > index 0341d51d6aed..5bc14dd70811 100644 > --- a/drivers/pci/controller/cadence/pci-j721e.c > +++ b/drivers/pci/controller/cadence/pci-j721e.c > @@ -644,7 +644,9 @@ static int j721e_pcie_suspend_noirq(struct device *dev) > struct j721e_pcie *pcie = dev_get_drvdata(dev); > > if (pcie->mode == PCI_MODE_RC) { > - gpiod_set_value_cansleep(pcie->reset_gpio, 0); > + if (pcie->reset_gpio) > + gpiod_set_value_cansleep(pcie->reset_gpio, 0); > + > clk_disable_unprepare(pcie->refclk); > } > Regards, Siddharth.
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? 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 > Fixes: c538d40f365b ("PCI: j721e: Add suspend and resume support") > Signed-off-by: Thomas Richard <thomas.richard@bootlin.com> > --- > drivers/pci/controller/cadence/pci-j721e.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c > index 0341d51d6aed..5bc14dd70811 100644 > --- a/drivers/pci/controller/cadence/pci-j721e.c > +++ b/drivers/pci/controller/cadence/pci-j721e.c > @@ -644,7 +644,9 @@ static int j721e_pcie_suspend_noirq(struct device *dev) > struct j721e_pcie *pcie = dev_get_drvdata(dev); > > if (pcie->mode == PCI_MODE_RC) { > - gpiod_set_value_cansleep(pcie->reset_gpio, 0); > + if (pcie->reset_gpio) > + gpiod_set_value_cansleep(pcie->reset_gpio, 0); > + > clk_disable_unprepare(pcie->refclk); > } > > -- > 2.39.5 >
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. Regards, Thomas
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
[+cc GPIO folks in case they think it's worthwhile to document that it's safe to pass NULL pointers to gpiod_*() interfaces] On Wed, Dec 11, 2024 at 02:44:21PM +0530, Manivannan Sadhasivam wrote: > 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. > > >> +++ b/drivers/pci/controller/cadence/pci-j721e.c > > >> @@ -644,7 +644,9 @@ static int j721e_pcie_suspend_noirq(struct device *dev) > > >> struct j721e_pcie *pcie = dev_get_drvdata(dev); > > >> > > >> if (pcie->mode == PCI_MODE_RC) { > > >> - gpiod_set_value_cansleep(pcie->reset_gpio, 0); > > >> + if (pcie->reset_gpio) > > >> + gpiod_set_value_cansleep(pcie->reset_gpio, 0); > > >> + > > >> clk_disable_unprepare(pcie->refclk); > > >> } > > > 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 > 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. I think it would be nice if the kernel-doc for these functions mentioned this somewhere. It's kind of a pain for every user to have to deduce this. Bjorn
diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c index 0341d51d6aed..5bc14dd70811 100644 --- a/drivers/pci/controller/cadence/pci-j721e.c +++ b/drivers/pci/controller/cadence/pci-j721e.c @@ -644,7 +644,9 @@ static int j721e_pcie_suspend_noirq(struct device *dev) struct j721e_pcie *pcie = dev_get_drvdata(dev); if (pcie->mode == PCI_MODE_RC) { - gpiod_set_value_cansleep(pcie->reset_gpio, 0); + if (pcie->reset_gpio) + gpiod_set_value_cansleep(pcie->reset_gpio, 0); + clk_disable_unprepare(pcie->refclk); }
The reset_gpio is optional, so in j721e_pcie_suspend_noirq() check if it is not NULL before to use it. Fixes: c538d40f365b ("PCI: j721e: Add suspend and resume support") Signed-off-by: Thomas Richard <thomas.richard@bootlin.com> --- drivers/pci/controller/cadence/pci-j721e.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)