Received-SPF: pass (google.com: domain of linux-kernel+bounces-42285-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Mon, 29 Jan 2024 08:31:39 +0100
From: Johan Hovold <johan@kernel.org>
To: VAMSHI GAJJELA <vamshigajjela@google.com>
Cc: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
	Stephen Boyd <sboyd@kernel.org>,
	Johan Hovold <johan+linaro@kernel.org>,
	Caleb Connolly <caleb.connolly@linaro.org>,
	linux-kernel@vger.kernel.org, manugautam@google.com
Subject: Re: [PATCH v1 RESEND] spmi: hisi-spmi-controller: Fix kernel panic
 on rmmod
Message-ID: <ZbdUW3GYVu6zF9QQ@hovoldconsulting.com>
References: <20240126061153.2883199-1-vamshigajjela@google.com>
 <ZbNq9q5mM6kRq7c3@hovoldconsulting.com>
 <CAMTSyjqOj64z3-mDm1pdQ5bi9ZM3j_cr7iSdCtvo40RyJrh38Q@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAMTSyjqOj64z3-mDm1pdQ5bi9ZM3j_cr7iSdCtvo40RyJrh38Q@mail.gmail.com>

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