diff mbox series

[v13,09/10] PCI: kirin: fix poweroff sequence

Message ID 8116a4ddaaeda8dd056e80fa0ee506c5c6f42ca7.1634539769.git.mchehab+huawei@kernel.org (mailing list archive)
State Superseded
Delegated to: Lorenzo Pieralisi
Headers show
Series Add support for Hikey 970 PCIe | expand

Commit Message

Mauro Carvalho Chehab Oct. 18, 2021, 7:07 a.m. UTC 
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 <songxiaowei@hisilicon.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---

See [PATCH v13 00/10] at: https://lore.kernel.org/all/cover.1634539769.git.mchehab+huawei@kernel.org/

 drivers/pci/controller/dwc/pcie-kirin.c | 30 ++++++++++++++++---------
 1 file changed, 20 insertions(+), 10 deletions(-)

Comments

Lorenzo Pieralisi Oct. 18, 2021, 10:21 a.m. UTC | #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 <songxiaowei@hisilicon.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
> 
> See [PATCH v13 00/10] at: https://lore.kernel.org/all/cover.1634539769.git.mchehab+huawei@kernel.org/
> 
>  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
>
Mauro Carvalho Chehab Oct. 18, 2021, 2:37 p.m. UTC | #2 
Em Mon, 18 Oct 2021 11:21:27 +0100
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> 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
stable@vger.kernel.org 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 <songxiaowei@hisilicon.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> > 
> > See [PATCH v13 00/10] at: https://lore.kernel.org/all/cover.1634539769.git.mchehab+huawei@kernel.org/
> > 
> >  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
Lorenzo Pieralisi Oct. 19, 2021, 9:40 a.m. UTC | #3 
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 <lorenzo.pieralisi@arm.com> 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
> stable@vger.kernel.org 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 <songxiaowei@hisilicon.com>
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > ---
> > > 
> > > See [PATCH v13 00/10] at: https://lore.kernel.org/all/cover.1634539769.git.mchehab+huawei@kernel.org/
> > > 
> > >  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
Lorenzo Pieralisi Oct. 21, 2021, 10:09 a.m. UTC | #4 
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 <lorenzo.pieralisi@arm.com> 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
> > stable@vger.kernel.org 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 <songxiaowei@hisilicon.com>
> > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > > ---
> > > > 
> > > > See [PATCH v13 00/10] at: https://lore.kernel.org/all/cover.1634539769.git.mchehab+huawei@kernel.org/
> > > > 
> > > >  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
diff mbox series

Patch

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;
 }