This driver currently doesn't call dw_pcie_host_deinit()
at the .remove() callback. This can cause an OOPS if the driver
is unbound.
While here, add a poweroff function, in order to abstract
between the internal and external PHY logic.
Acked-by: Xiaowei Song <[email protected]>
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
See [PATCH v13 00/10] at: https://lore.kernel.org/all/[email protected]/
drivers/pci/controller/dwc/pcie-kirin.c | 30 ++++++++++++++++---------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index b17a194cf78d..ffc63d12f8ed 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -680,6 +680,23 @@ static const struct dw_pcie_host_ops kirin_pcie_host_ops = {
.host_init = kirin_pcie_host_init,
};
+static int kirin_pcie_power_off(struct kirin_pcie *kirin_pcie)
+{
+ int i;
+
+ if (kirin_pcie->type == PCIE_KIRIN_INTERNAL_PHY)
+ return hi3660_pcie_phy_power_off(kirin_pcie);
+
+ for (i = 0; i < kirin_pcie->n_gpio_clkreq; i++) {
+ gpio_direction_output(kirin_pcie->gpio_id_clkreq[i], 1);
+ }
+
+ phy_power_off(kirin_pcie->phy);
+ phy_exit(kirin_pcie->phy);
+
+ return 0;
+}
+
static int kirin_pcie_power_on(struct platform_device *pdev,
struct kirin_pcie *kirin_pcie)
{
@@ -725,12 +742,7 @@ static int kirin_pcie_power_on(struct platform_device *pdev,
return 0;
err:
- if (kirin_pcie->type == PCIE_KIRIN_INTERNAL_PHY) {
- hi3660_pcie_phy_power_off(kirin_pcie);
- } else {
- phy_power_off(kirin_pcie->phy);
- phy_exit(kirin_pcie->phy);
- }
+ kirin_pcie_power_off(kirin_pcie);
return ret;
}
@@ -739,11 +751,9 @@ static int __exit kirin_pcie_remove(struct platform_device *pdev)
{
struct kirin_pcie *kirin_pcie = platform_get_drvdata(pdev);
- if (kirin_pcie->type == PCIE_KIRIN_INTERNAL_PHY)
- return hi3660_pcie_phy_power_off(kirin_pcie);
+ dw_pcie_host_deinit(&kirin_pcie->pci->pp);
- phy_power_off(kirin_pcie->phy);
- phy_exit(kirin_pcie->phy);
+ kirin_pcie_power_off(kirin_pcie);
return 0;
}
--
2.31.1
On Mon, Oct 18, 2021 at 08:07:34AM +0100, Mauro Carvalho Chehab wrote:
> This driver currently doesn't call dw_pcie_host_deinit()
> at the .remove() callback. This can cause an OOPS if the driver
> is unbound.
This looks like a fix, it has to be marked as such.
> While here, add a poweroff function, in order to abstract
> between the internal and external PHY logic.
>
> Acked-by: Xiaowei Song <[email protected]>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> ---
>
> See [PATCH v13 00/10] at: https://lore.kernel.org/all/[email protected]/
>
> drivers/pci/controller/dwc/pcie-kirin.c | 30 ++++++++++++++++---------
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> index b17a194cf78d..ffc63d12f8ed 100644
> --- a/drivers/pci/controller/dwc/pcie-kirin.c
> +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> @@ -680,6 +680,23 @@ static const struct dw_pcie_host_ops kirin_pcie_host_ops = {
> .host_init = kirin_pcie_host_init,
> };
>
> +static int kirin_pcie_power_off(struct kirin_pcie *kirin_pcie)
> +{
> + int i;
> +
> + if (kirin_pcie->type == PCIE_KIRIN_INTERNAL_PHY)
> + return hi3660_pcie_phy_power_off(kirin_pcie);
> +
> + for (i = 0; i < kirin_pcie->n_gpio_clkreq; i++) {
> + gpio_direction_output(kirin_pcie->gpio_id_clkreq[i], 1);
> + }
It looks like you are adding functionality here (ie gpio), not
just wrapping common code in a function.
Also, remove the braces, they aren't needed.
Lorenzo
> +
> + phy_power_off(kirin_pcie->phy);
> + phy_exit(kirin_pcie->phy);
> +
> + return 0;
> +}
> +
> static int kirin_pcie_power_on(struct platform_device *pdev,
> struct kirin_pcie *kirin_pcie)
> {
> @@ -725,12 +742,7 @@ static int kirin_pcie_power_on(struct platform_device *pdev,
>
> return 0;
> err:
> - if (kirin_pcie->type == PCIE_KIRIN_INTERNAL_PHY) {
> - hi3660_pcie_phy_power_off(kirin_pcie);
> - } else {
> - phy_power_off(kirin_pcie->phy);
> - phy_exit(kirin_pcie->phy);
> - }
> + kirin_pcie_power_off(kirin_pcie);
>
> return ret;
> }
> @@ -739,11 +751,9 @@ static int __exit kirin_pcie_remove(struct platform_device *pdev)
> {
> struct kirin_pcie *kirin_pcie = platform_get_drvdata(pdev);
>
> - if (kirin_pcie->type == PCIE_KIRIN_INTERNAL_PHY)
> - return hi3660_pcie_phy_power_off(kirin_pcie);
> + dw_pcie_host_deinit(&kirin_pcie->pci->pp);
>
> - phy_power_off(kirin_pcie->phy);
> - phy_exit(kirin_pcie->phy);
> + kirin_pcie_power_off(kirin_pcie);
>
> return 0;
> }
> --
> 2.31.1
>
Em Mon, 18 Oct 2021 11:21:27 +0100
Lorenzo Pieralisi <[email protected]> escreveu:
> On Mon, Oct 18, 2021 at 08:07:34AM +0100, Mauro Carvalho Chehab wrote:
> > This driver currently doesn't call dw_pcie_host_deinit()
> > at the .remove() callback. This can cause an OOPS if the driver
> > is unbound.
>
> This looks like a fix, it has to be marked as such.
Well, without patch 10/10, the .remove() ops won't be called,
so, it is not really a fix, but I can surely add a c/c
[email protected] and add a Fixes: tag here.
>
> > While here, add a poweroff function, in order to abstract
> > between the internal and external PHY logic.
> >
> > Acked-by: Xiaowei Song <[email protected]>
> > Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> > ---
> >
> > See [PATCH v13 00/10] at: https://lore.kernel.org/all/[email protected]/
> >
> > drivers/pci/controller/dwc/pcie-kirin.c | 30 ++++++++++++++++---------
> > 1 file changed, 20 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> > index b17a194cf78d..ffc63d12f8ed 100644
> > --- a/drivers/pci/controller/dwc/pcie-kirin.c
> > +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> > @@ -680,6 +680,23 @@ static const struct dw_pcie_host_ops kirin_pcie_host_ops = {
> > .host_init = kirin_pcie_host_init,
> > };
> >
> > +static int kirin_pcie_power_off(struct kirin_pcie *kirin_pcie)
> > +{
> > + int i;
> > +
> > + if (kirin_pcie->type == PCIE_KIRIN_INTERNAL_PHY)
> > + return hi3660_pcie_phy_power_off(kirin_pcie);
> > +
> > + for (i = 0; i < kirin_pcie->n_gpio_clkreq; i++) {
> > + gpio_direction_output(kirin_pcie->gpio_id_clkreq[i], 1);
> > + }
>
> It looks like you are adding functionality here (ie gpio), not
> just wrapping common code in a function.
It is just reverting the power on logic there.
>
> Also, remove the braces, they aren't needed.
Yeah, I forgot to drop it, when I dropped a tem code that had some
dev_dbg() on it.
I'll drop on v14.
Regards,
Mauro
On Mon, Oct 18, 2021 at 03:37:16PM +0100, Mauro Carvalho Chehab wrote:
> Em Mon, 18 Oct 2021 11:21:27 +0100
> Lorenzo Pieralisi <[email protected]> escreveu:
>
> > On Mon, Oct 18, 2021 at 08:07:34AM +0100, Mauro Carvalho Chehab wrote:
> > > This driver currently doesn't call dw_pcie_host_deinit()
> > > at the .remove() callback. This can cause an OOPS if the driver
> > > is unbound.
> >
> > This looks like a fix, it has to be marked as such.
>
> Well, without patch 10/10, the .remove() ops won't be called,
> so, it is not really a fix, but I can surely add a c/c
> [email protected] and add a Fixes: tag here.
You have a point - unless we send patch 10 to stable as well I
would not tag it then.
> > > While here, add a poweroff function, in order to abstract
> > > between the internal and external PHY logic.
> > >
> > > Acked-by: Xiaowei Song <[email protected]>
> > > Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> > > ---
> > >
> > > See [PATCH v13 00/10] at: https://lore.kernel.org/all/[email protected]/
> > >
> > > drivers/pci/controller/dwc/pcie-kirin.c | 30 ++++++++++++++++---------
> > > 1 file changed, 20 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> > > index b17a194cf78d..ffc63d12f8ed 100644
> > > --- a/drivers/pci/controller/dwc/pcie-kirin.c
> > > +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> > > @@ -680,6 +680,23 @@ static const struct dw_pcie_host_ops kirin_pcie_host_ops = {
> > > .host_init = kirin_pcie_host_init,
> > > };
> > >
> > > +static int kirin_pcie_power_off(struct kirin_pcie *kirin_pcie)
> > > +{
> > > + int i;
> > > +
> > > + if (kirin_pcie->type == PCIE_KIRIN_INTERNAL_PHY)
> > > + return hi3660_pcie_phy_power_off(kirin_pcie);
> > > +
> > > + for (i = 0; i < kirin_pcie->n_gpio_clkreq; i++) {
> > > + gpio_direction_output(kirin_pcie->gpio_id_clkreq[i], 1);
> > > + }
> >
> > It looks like you are adding functionality here (ie gpio), not
> > just wrapping common code in a function.
>
> It is just reverting the power on logic there.
What I am saying is that executing:
for (i = 0; i < kirin_pcie->n_gpio_clkreq; i++)
gpio_direction_output(kirin_pcie->gpio_id_clkreq[i], 1);
is an addition to what current code does AFAICS (ie you are not just
moving code into a function - kirin_pcie_power_off(), you are adding
to it), it is a logical change that belongs in a separate patch.
There are two logical changes:
- Adding dw_pcie_host_deinit()
- Moving PHY power off code into kirin_pcie_power_off() (and adding
gpio handling in it)
That's what I read from the diffstat, please correct me if I am wrong.
Thanks,
Lorenzo
> >
> > Also, remove the braces, they aren't needed.
>
> Yeah, I forgot to drop it, when I dropped a tem code that had some
> dev_dbg() on it.
>
> I'll drop on v14.
>
> Regards,
> Mauro
On Tue, Oct 19, 2021 at 10:40:48AM +0100, Lorenzo Pieralisi wrote:
> On Mon, Oct 18, 2021 at 03:37:16PM +0100, Mauro Carvalho Chehab wrote:
> > Em Mon, 18 Oct 2021 11:21:27 +0100
> > Lorenzo Pieralisi <[email protected]> escreveu:
> >
> > > On Mon, Oct 18, 2021 at 08:07:34AM +0100, Mauro Carvalho Chehab wrote:
> > > > This driver currently doesn't call dw_pcie_host_deinit()
> > > > at the .remove() callback. This can cause an OOPS if the driver
> > > > is unbound.
> > >
> > > This looks like a fix, it has to be marked as such.
> >
> > Well, without patch 10/10, the .remove() ops won't be called,
> > so, it is not really a fix, but I can surely add a c/c
> > [email protected] and add a Fixes: tag here.
>
> You have a point - unless we send patch 10 to stable as well I
> would not tag it then.
>
> > > > While here, add a poweroff function, in order to abstract
> > > > between the internal and external PHY logic.
> > > >
> > > > Acked-by: Xiaowei Song <[email protected]>
> > > > Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> > > > ---
> > > >
> > > > See [PATCH v13 00/10] at: https://lore.kernel.org/all/[email protected]/
> > > >
> > > > drivers/pci/controller/dwc/pcie-kirin.c | 30 ++++++++++++++++---------
> > > > 1 file changed, 20 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> > > > index b17a194cf78d..ffc63d12f8ed 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-kirin.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> > > > @@ -680,6 +680,23 @@ static const struct dw_pcie_host_ops kirin_pcie_host_ops = {
> > > > .host_init = kirin_pcie_host_init,
> > > > };
> > > >
> > > > +static int kirin_pcie_power_off(struct kirin_pcie *kirin_pcie)
> > > > +{
> > > > + int i;
> > > > +
> > > > + if (kirin_pcie->type == PCIE_KIRIN_INTERNAL_PHY)
> > > > + return hi3660_pcie_phy_power_off(kirin_pcie);
> > > > +
> > > > + for (i = 0; i < kirin_pcie->n_gpio_clkreq; i++) {
> > > > + gpio_direction_output(kirin_pcie->gpio_id_clkreq[i], 1);
> > > > + }
> > >
> > > It looks like you are adding functionality here (ie gpio), not
> > > just wrapping common code in a function.
> >
> > It is just reverting the power on logic there.
>
> What I am saying is that executing:
>
> for (i = 0; i < kirin_pcie->n_gpio_clkreq; i++)
> gpio_direction_output(kirin_pcie->gpio_id_clkreq[i], 1);
>
> is an addition to what current code does AFAICS (ie you are not just
> moving code into a function - kirin_pcie_power_off(), you are adding
> to it), it is a logical change that belongs in a separate patch.
>
> There are two logical changes:
>
> - Adding dw_pcie_host_deinit()
> - Moving PHY power off code into kirin_pcie_power_off() (and adding
> gpio handling in it)
>
> That's what I read from the diffstat, please correct me if I am wrong.
Hi Mauro,
any comment on the above ? It is the last question I have before
merging the series, please let me know.
Thanks,
Lorenzo