From: Bjorn Helgaas <helgaas@kernel.org> To: Philipp Zabel <p.zabel@pengutronix.de> Cc: linux-pci@vger.kernel.org, Bjorn Helgaas <bhelgaas@google.com>, Matthias Brugger <matthias.bgg@gmail.com>, Ryder Lee <ryder.lee@mediatek.com>, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>, Rob Herring <robh@kernel.org>, linux-mediatek@lists.infradead.org Subject: Re: [PATCH] PCI: mediatek: fix optional reset handling Date: Fri, 5 Mar 2021 09:36:59 -0600 [thread overview] Message-ID: <20210305153659.GA1098921@bjorn-Precision-5520> (raw) In-Reply-To: <20210305091715.4319-1-p.zabel@pengutronix.de> [+cc Ryder, Lorenzo, Rob, linux-mediatek (mediatek maintainers)] Capitalize first word of subject per convention: PCI: mediatek: Fix optional reset handling But "Fix" is pretty non-specific; it doesn't give any information about what the change does. On Fri, Mar 05, 2021 at 10:17:15AM +0100, Philipp Zabel wrote: > As of commit bb475230b8e5 ("reset: make optional functions really > optional"), the reset framework API calls use NULL pointers to describe > optional, non-present reset controls. > > This allows to unconditionally return errors from > devm_reset_control_get_optional_exclusive. "devm_reset_control_get_optional_exclusive()" This basically restates the code change, but doesn't say *why* we want this change. > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > drivers/pci/controller/pcie-mediatek.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c > index 23548b517e4b..35c66fa770a6 100644 > --- a/drivers/pci/controller/pcie-mediatek.c > +++ b/drivers/pci/controller/pcie-mediatek.c > @@ -954,7 +954,7 @@ static int mtk_pcie_parse_port(struct mtk_pcie *pcie, > > snprintf(name, sizeof(name), "pcie-rst%d", slot); > port->reset = devm_reset_control_get_optional_exclusive(dev, name); > - if (PTR_ERR(port->reset) == -EPROBE_DEFER) > + if (IS_ERR(port->reset)) > return PTR_ERR(port->reset); Before this patch, -EPROBE_DEFER: abort probe, return -EPROBE_DEFER other errors: port->reset = -EINVAL, etc, continue probe future WARN_ON() from reset_control_assert() NULL (no reset control): port->reset = NULL, continue probe valid reset control: port->reset = rstc, continue probe After this patch, all errors: abort probe, return err NULL (no reset control): port->reset = NULL, continue probe valid reset control: port->reset = rstc, continue probe So IIUC, if devm_reset_control_get_optional_exclusive() returns an error other than -EPROBE_DEFER, e.g., -EINVAL, we used to continue. We would then get warnings from reset_control_assert() and reset_control_deassert() because "IS_ERR(port->reset)" was true. Since the reset control seems to be optional, I assume the port *may* still be usable even though we get warnings. But after this patch, if devm_reset_control_get_optional_exclusive() returns -EINVAL, we will fail the probe, rendering the port useless. If my understanding is correct, this seems like a difference we should mention in the commit log because it took me a while to untangle this and the subject line doesn't hint at it at all. > /* some platforms may use default PHY setting */ > -- > 2.29.2 >
WARNING: multiple messages have this Message-ID (diff)
From: Bjorn Helgaas <helgaas@kernel.org> To: Philipp Zabel <p.zabel@pengutronix.de> Cc: linux-pci@vger.kernel.org, Bjorn Helgaas <bhelgaas@google.com>, Matthias Brugger <matthias.bgg@gmail.com>, Ryder Lee <ryder.lee@mediatek.com>, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>, Rob Herring <robh@kernel.org>, linux-mediatek@lists.infradead.org Subject: Re: [PATCH] PCI: mediatek: fix optional reset handling Date: Fri, 5 Mar 2021 09:36:59 -0600 [thread overview] Message-ID: <20210305153659.GA1098921@bjorn-Precision-5520> (raw) In-Reply-To: <20210305091715.4319-1-p.zabel@pengutronix.de> [+cc Ryder, Lorenzo, Rob, linux-mediatek (mediatek maintainers)] Capitalize first word of subject per convention: PCI: mediatek: Fix optional reset handling But "Fix" is pretty non-specific; it doesn't give any information about what the change does. On Fri, Mar 05, 2021 at 10:17:15AM +0100, Philipp Zabel wrote: > As of commit bb475230b8e5 ("reset: make optional functions really > optional"), the reset framework API calls use NULL pointers to describe > optional, non-present reset controls. > > This allows to unconditionally return errors from > devm_reset_control_get_optional_exclusive. "devm_reset_control_get_optional_exclusive()" This basically restates the code change, but doesn't say *why* we want this change. > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > drivers/pci/controller/pcie-mediatek.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c > index 23548b517e4b..35c66fa770a6 100644 > --- a/drivers/pci/controller/pcie-mediatek.c > +++ b/drivers/pci/controller/pcie-mediatek.c > @@ -954,7 +954,7 @@ static int mtk_pcie_parse_port(struct mtk_pcie *pcie, > > snprintf(name, sizeof(name), "pcie-rst%d", slot); > port->reset = devm_reset_control_get_optional_exclusive(dev, name); > - if (PTR_ERR(port->reset) == -EPROBE_DEFER) > + if (IS_ERR(port->reset)) > return PTR_ERR(port->reset); Before this patch, -EPROBE_DEFER: abort probe, return -EPROBE_DEFER other errors: port->reset = -EINVAL, etc, continue probe future WARN_ON() from reset_control_assert() NULL (no reset control): port->reset = NULL, continue probe valid reset control: port->reset = rstc, continue probe After this patch, all errors: abort probe, return err NULL (no reset control): port->reset = NULL, continue probe valid reset control: port->reset = rstc, continue probe So IIUC, if devm_reset_control_get_optional_exclusive() returns an error other than -EPROBE_DEFER, e.g., -EINVAL, we used to continue. We would then get warnings from reset_control_assert() and reset_control_deassert() because "IS_ERR(port->reset)" was true. Since the reset control seems to be optional, I assume the port *may* still be usable even though we get warnings. But after this patch, if devm_reset_control_get_optional_exclusive() returns -EINVAL, we will fail the probe, rendering the port useless. If my understanding is correct, this seems like a difference we should mention in the commit log because it took me a while to untangle this and the subject line doesn't hint at it at all. > /* some platforms may use default PHY setting */ > -- > 2.29.2 > _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek
next prev parent reply other threads:[~2021-03-05 15:37 UTC|newest] Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-05 9:17 [PATCH] PCI: mediatek: fix optional reset handling Philipp Zabel 2021-03-05 15:36 ` Bjorn Helgaas [this message] 2021-03-05 15:36 ` Bjorn Helgaas
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210305153659.GA1098921@bjorn-Precision-5520 \ --to=helgaas@kernel.org \ --cc=bhelgaas@google.com \ --cc=linux-mediatek@lists.infradead.org \ --cc=linux-pci@vger.kernel.org \ --cc=lorenzo.pieralisi@arm.com \ --cc=matthias.bgg@gmail.com \ --cc=p.zabel@pengutronix.de \ --cc=robh@kernel.org \ --cc=ryder.lee@mediatek.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.