diff mbox series

[v1,RESEND] spmi: hisi-spmi-controller: Fix kernel panic on rmmod

Message ID 20240126061153.2883199-1-vamshigajjela@google.com
State New
Headers
Series [v1,RESEND] spmi: hisi-spmi-controller: Fix kernel panic on rmmod |

Commit Message

Vamshi Gajjela Jan. 26, 2024, 6:11 a.m. UTC 
  Ensure consistency in spmi_controller pointers between
spmi_controller_remove/put and driver spmi_del_controller functions.
The former requires a pointer to struct spmi_controller, while the
latter passes a pointer of struct spmi_controller_dev, leading to a
"Null pointer exception".

'nr' member of struct spmi_controller, which serves as an identifier
for the controller/bus. This value is assigned a dynamic ID in
spmi_controller_alloc, and overriding it from the driver results in an
ida_free error "ida_free called for id=xx which is not allocated".

Signed-off-by: Vamshi Gajjela <vamshigajjela@google.com>
---
 drivers/spmi/hisi-spmi-controller.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Johan Hovold Jan. 26, 2024, 8:19 a.m. UTC | #1 
On Fri, Jan 26, 2024 at 11:41:53AM +0530, Vamshi Gajjela wrote:
> Ensure consistency in spmi_controller pointers between
> spmi_controller_remove/put and driver spmi_del_controller functions.
> The former requires a pointer to struct spmi_controller, while the
> latter passes a pointer of struct spmi_controller_dev, leading to a
> "Null pointer exception".
> 
> 'nr' member of struct spmi_controller, which serves as an identifier
> for the controller/bus. This value is assigned a dynamic ID in
> spmi_controller_alloc, and overriding it from the driver results in an
> ida_free error "ida_free called for id=xx which is not allocated".

No Fixes tag?

> Signed-off-by: Vamshi Gajjela <vamshigajjela@google.com>
> ---
>  drivers/spmi/hisi-spmi-controller.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spmi/hisi-spmi-controller.c b/drivers/spmi/hisi-spmi-controller.c
> index 9cbd473487cb..af51ffe24072 100644
> --- a/drivers/spmi/hisi-spmi-controller.c
> +++ b/drivers/spmi/hisi-spmi-controller.c
> @@ -303,7 +303,6 @@ static int spmi_controller_probe(struct platform_device *pdev)
>  
>  	spin_lock_init(&spmi_controller->lock);
>  
> -	ctrl->nr = spmi_controller->channel;
>  	ctrl->dev.parent = pdev->dev.parent;
>  	ctrl->dev.of_node = of_node_get(pdev->dev.of_node);
>  
> @@ -326,7 +325,8 @@ static int spmi_controller_probe(struct platform_device *pdev)
>  
>  static void spmi_del_controller(struct platform_device *pdev)

This function does not exist in mainline so presumably this is some bug
you've introduced in your downstream driver that you're trying to fix.

So this patch looks all bogus.

>  {
> -	struct spmi_controller *ctrl = platform_get_drvdata(pdev);
> +	struct spmi_controller_dev *spmi_controller = platform_get_drvdata(pdev);
> +	struct spmi_controller *ctrl = spmi_controller->controller;
>  
>  	spmi_controller_remove(ctrl);
>  	spmi_controller_put(ctrl);

Johan
Vamshi Gajjela Jan. 29, 2024, 4:43 a.m. UTC | #2 
On Fri, Jan 26, 2024 at 1:48 PM Johan Hovold <johan@kernel.org> wrote:
>
> On Fri, Jan 26, 2024 at 11:41:53AM +0530, Vamshi Gajjela wrote:
> > Ensure consistency in spmi_controller pointers between
> > spmi_controller_remove/put and driver spmi_del_controller functions.
> > The former requires a pointer to struct spmi_controller, while the
> > latter passes a pointer of struct spmi_controller_dev, leading to a
> > "Null pointer exception".
> >
> > 'nr' member of struct spmi_controller, which serves as an identifier
> > for the controller/bus. This value is assigned a dynamic ID in
> > spmi_controller_alloc, and overriding it from the driver results in an
> > ida_free error "ida_free called for id=xx which is not allocated".
>
> No Fixes tag?
There isn't a bug, I will remove word "Fix"
>
> > Signed-off-by: Vamshi Gajjela <vamshigajjela@google.com>
> > ---
> >  drivers/spmi/hisi-spmi-controller.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/spmi/hisi-spmi-controller.c b/drivers/spmi/hisi-spmi-controller.c
> > index 9cbd473487cb..af51ffe24072 100644
> > --- a/drivers/spmi/hisi-spmi-controller.c
> > +++ b/drivers/spmi/hisi-spmi-controller.c
> > @@ -303,7 +303,6 @@ static int spmi_controller_probe(struct platform_device *pdev)
> >
> >       spin_lock_init(&spmi_controller->lock);
> >
> > -     ctrl->nr = spmi_controller->channel;
This remains applicable, however, it could lead to a failure in the
spmi_ctrl_release, I
will refactor the patch to address this.
also "spmi_del_controller" is removed from 6.7.2
> >       ctrl->dev.parent = pdev->dev.parent;
> >       ctrl->dev.of_node = of_node_get(pdev->dev.of_node);
> >
> > @@ -326,7 +325,8 @@ static int spmi_controller_probe(struct platform_device *pdev)
> >
> >  static void spmi_del_controller(struct platform_device *pdev)
>
> This function does not exist in mainline so presumably this is some bug
> you've introduced in your downstream driver that you're trying to fix.
>
> So this patch looks all bogus.
spmi_del_controller is present until in 6.7.2, I have made this patch
in last week of Dec,
I should have checked before resending, apologies.


>
> >  {
> > -     struct spmi_controller *ctrl = platform_get_drvdata(pdev);
> > +     struct spmi_controller_dev *spmi_controller = platform_get_drvdata(pdev);
> > +     struct spmi_controller *ctrl = spmi_controller->controller;
> >
> >       spmi_controller_remove(ctrl);
> >       spmi_controller_put(ctrl);
>
> Johan
Johan Hovold Jan. 29, 2024, 7:31 a.m. UTC | #3 
On Mon, Jan 29, 2024 at 10:13:22AM +0530, VAMSHI GAJJELA wrote:
> On Fri, Jan 26, 2024 at 1:48 PM Johan Hovold <johan@kernel.org> wrote:
> >
> > On Fri, Jan 26, 2024 at 11:41:53AM +0530, Vamshi Gajjela wrote:
> > > Ensure consistency in spmi_controller pointers between
> > > spmi_controller_remove/put and driver spmi_del_controller functions.
> > > The former requires a pointer to struct spmi_controller, while the
> > > latter passes a pointer of struct spmi_controller_dev, leading to a
> > > "Null pointer exception".
> > >
> > > 'nr' member of struct spmi_controller, which serves as an identifier
> > > for the controller/bus. This value is assigned a dynamic ID in
> > > spmi_controller_alloc, and overriding it from the driver results in an
> > > ida_free error "ida_free called for id=xx which is not allocated".
> >
> > No Fixes tag?

> There isn't a bug, I will remove word "Fix"

Both of the issues you point out above sounds like bugs that deserve a
Fixes tag.

> > > Signed-off-by: Vamshi Gajjela <vamshigajjela@google.com>
> > > ---
> > >  drivers/spmi/hisi-spmi-controller.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/spmi/hisi-spmi-controller.c b/drivers/spmi/hisi-spmi-controller.c
> > > index 9cbd473487cb..af51ffe24072 100644
> > > --- a/drivers/spmi/hisi-spmi-controller.c
> > > +++ b/drivers/spmi/hisi-spmi-controller.c
> > > @@ -303,7 +303,6 @@ static int spmi_controller_probe(struct platform_device *pdev)
> > >
> > >       spin_lock_init(&spmi_controller->lock);
> > >
> > > -     ctrl->nr = spmi_controller->channel;

> This remains applicable, however, it could lead to a failure in the
> spmi_ctrl_release, I
> will refactor the patch to address this.
> also "spmi_del_controller" is removed from 6.7.2

No, this has not changed in 6.7.2, it has been removed from 6.8-rc1.

> > >       ctrl->dev.parent = pdev->dev.parent;
> > >       ctrl->dev.of_node = of_node_get(pdev->dev.of_node);
> > >
> > > @@ -326,7 +325,8 @@ static int spmi_controller_probe(struct platform_device *pdev)
> > >
> > >  static void spmi_del_controller(struct platform_device *pdev)
> >
> > This function does not exist in mainline so presumably this is some bug
> > you've introduced in your downstream driver that you're trying to fix.
> >
> > So this patch looks all bogus.

> spmi_del_controller is present until in 6.7.2, I have made this patch
> in last week of Dec,
> I should have checked before resending, apologies.

The bug you found was apparently accidentally fixed by commit
490d88ef548d ("spmi: hisi-spmi-controller: Use
devm_spmi_controller_add()") in 6.8-rc1 but I don't see any record of it
having been backported yet.

As it depends on new helper function that will likely not happen either.

Perhaps you can split your patch in two separate fixes and ask the stable
team to backport the driver-data one.

> > >  {
> > > -     struct spmi_controller *ctrl = platform_get_drvdata(pdev);
> > > +     struct spmi_controller_dev *spmi_controller = platform_get_drvdata(pdev);
> > > +     struct spmi_controller *ctrl = spmi_controller->controller;
> > >
> > >       spmi_controller_remove(ctrl);
> > >       spmi_controller_put(ctrl);

Johan
Vamshi Gajjela Feb. 2, 2024, 11:59 a.m. UTC | #4 
On Mon, Jan 29, 2024 at 1:01 PM Johan Hovold <johan@kernel.org> wrote:
>
> On Mon, Jan 29, 2024 at 10:13:22AM +0530, VAMSHI GAJJELA wrote:
> > On Fri, Jan 26, 2024 at 1:48 PM Johan Hovold <johan@kernel.org> wrote:
> > >
> > > On Fri, Jan 26, 2024 at 11:41:53AM +0530, Vamshi Gajjela wrote:
> > > > Ensure consistency in spmi_controller pointers between
> > > > spmi_controller_remove/put and driver spmi_del_controller functions.
> > > > The former requires a pointer to struct spmi_controller, while the
> > > > latter passes a pointer of struct spmi_controller_dev, leading to a
> > > > "Null pointer exception".
> > > >
> > > > 'nr' member of struct spmi_controller, which serves as an identifier
> > > > for the controller/bus. This value is assigned a dynamic ID in
> > > > spmi_controller_alloc, and overriding it from the driver results in an
> > > > ida_free error "ida_free called for id=xx which is not allocated".
> > >
> > > No Fixes tag?
>
> > There isn't a bug, I will remove word "Fix"
>
> Both of the issues you point out above sounds like bugs that deserve a
> Fixes tag.
ACK
>
> > > > Signed-off-by: Vamshi Gajjela <vamshigajjela@google.com>
> > > > ---
> > > >  drivers/spmi/hisi-spmi-controller.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/spmi/hisi-spmi-controller.c b/drivers/spmi/hisi-spmi-controller.c
> > > > index 9cbd473487cb..af51ffe24072 100644
> > > > --- a/drivers/spmi/hisi-spmi-controller.c
> > > > +++ b/drivers/spmi/hisi-spmi-controller.c
> > > > @@ -303,7 +303,6 @@ static int spmi_controller_probe(struct platform_device *pdev)
> > > >
> > > >       spin_lock_init(&spmi_controller->lock);
> > > >
> > > > -     ctrl->nr = spmi_controller->channel;
>
> > This remains applicable, however, it could lead to a failure in the
> > spmi_ctrl_release, I
> > will refactor the patch to address this.
> > also "spmi_del_controller" is removed from 6.7.2
>
> No, this has not changed in 6.7.2, it has been removed from 6.8-rc1.
ACK
>
> > > >       ctrl->dev.parent = pdev->dev.parent;
> > > >       ctrl->dev.of_node = of_node_get(pdev->dev.of_node);
> > > >
> > > > @@ -326,7 +325,8 @@ static int spmi_controller_probe(struct platform_device *pdev)
> > > >
> > > >  static void spmi_del_controller(struct platform_device *pdev)
> > >
> > > This function does not exist in mainline so presumably this is some bug
> > > you've introduced in your downstream driver that you're trying to fix.
> > >
> > > So this patch looks all bogus.
>
> > spmi_del_controller is present until in 6.7.2, I have made this patch
> > in last week of Dec,
> > I should have checked before resending, apologies.
>
> The bug you found was apparently accidentally fixed by commit
> 490d88ef548d ("spmi: hisi-spmi-controller: Use
> devm_spmi_controller_add()") in 6.8-rc1 but I don't see any record of it
> having been backported yet.
>
> As it depends on new helper function that will likely not happen either.
>
> Perhaps you can split your patch in two separate fixes and ask the stable
> team to backport the driver-data one.
Sure, I will split this into two as suggested, Thanks.
>
> > > >  {
> > > > -     struct spmi_controller *ctrl = platform_get_drvdata(pdev);
> > > > +     struct spmi_controller_dev *spmi_controller = platform_get_drvdata(pdev);
> > > > +     struct spmi_controller *ctrl = spmi_controller->controller;
> > > >
> > > >       spmi_controller_remove(ctrl);
> > > >       spmi_controller_put(ctrl);
>
> Johan
diff mbox series

Patch

diff --git a/drivers/spmi/hisi-spmi-controller.c b/drivers/spmi/hisi-spmi-controller.c
index 9cbd473487cb..af51ffe24072 100644
--- a/drivers/spmi/hisi-spmi-controller.c
+++ b/drivers/spmi/hisi-spmi-controller.c
@@ -303,7 +303,6 @@  static int spmi_controller_probe(struct platform_device *pdev)
 
 	spin_lock_init(&spmi_controller->lock);
 
-	ctrl->nr = spmi_controller->channel;
 	ctrl->dev.parent = pdev->dev.parent;
 	ctrl->dev.of_node = of_node_get(pdev->dev.of_node);
 
@@ -326,7 +325,8 @@  static int spmi_controller_probe(struct platform_device *pdev)
 
 static void spmi_del_controller(struct platform_device *pdev)
 {
-	struct spmi_controller *ctrl = platform_get_drvdata(pdev);
+	struct spmi_controller_dev *spmi_controller = platform_get_drvdata(pdev);
+	struct spmi_controller *ctrl = spmi_controller->controller;
 
 	spmi_controller_remove(ctrl);
 	spmi_controller_put(ctrl);