Re: [PATCH v4 1/2] soc: qcom: aoss: Expose send for generic usecase

[Deepak Kumar Singh]

On 7/21/2021 12:07 PM, Stephen Boyd wrote:
> Quoting Sibi Sankar (2021-06-09 04:18:51)
> > From: Deepak Kumar Singh <deesin@xxxxxxxxxxxxxx>
> >
> > Not all upcoming usecases will have an interface to allow the aoss
> > driver to hook onto. Expose the send api and create a get function to
> > enable drivers to send their own messages to aoss.
> >
> > Signed-off-by: Chris Lew <clew@xxxxxxxxxxxxxx>
> > Signed-off-by: Deepak Kumar Singh <deesin@xxxxxxxxxxxxxx>
> > Reviewed-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> > Signed-off-by: Sibi Sankar <sibis@xxxxxxxxxxxxxx>
> > ---
> >
> > v4:
> >   * Fix compilation error due to missing qmp_put
> >   * Minor typos [s/tarcks/tracks]
> >
> >   drivers/soc/qcom/qcom_aoss.c       | 70 ++++++++++++++++++++++++++++++++++++--
> >   include/linux/soc/qcom/qcom_aoss.h | 36 ++++++++++++++++++++
> >   2 files changed, 104 insertions(+), 2 deletions(-)
> >   create mode 100644 include/linux/soc/qcom/qcom_aoss.h
> >
> > diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
> > index 934fcc4d2b05..e8f48760bac8 100644
> > --- a/drivers/soc/qcom/qcom_aoss.c
> > +++ b/drivers/soc/qcom/qcom_aoss.c
> > @@ -522,13 +582,14 @@ static int qmp_probe(struct platform_device *pdev)
> >          int irq;
> >          int ret;
> >
> > -       qmp = devm_kzalloc(&pdev->dev, sizeof(*qmp), GFP_KERNEL);
> > +       qmp = kzalloc(sizeof(*qmp), GFP_KERNEL);
> >          if (!qmp)
> >                  return -ENOMEM;
> >
> >          qmp->dev = &pdev->dev;
> >          init_waitqueue_head(&qmp->event);
> >          mutex_init(&qmp->tx_lock);
> > +       kref_init(&qmp->refcount);
> >
> >          res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >          qmp->msgram = devm_ioremap_resource(&pdev->dev, res);
> > @@ -569,6 +630,8 @@ static int qmp_probe(struct platform_device *pdev)
> >
> >          platform_set_drvdata(pdev, qmp);
> >
> > +       atomic_set(&qmp->orphan, 0);
> > +
> >          return 0;
> >
> >   err_remove_qdss_clk:
> > @@ -577,6 +640,7 @@ static int qmp_probe(struct platform_device *pdev)
> >          qmp_close(qmp);
> >   err_free_mbox:
> >          mbox_free_channel(qmp->mbox_chan);
> > +       kfree(qmp);
> >
> >          return ret;
> >   }
> > @@ -590,7 +654,9 @@ static int qmp_remove(struct platform_device *pdev)
> >          qmp_cooling_devices_remove(qmp);
> >
> >          qmp_close(qmp);
> > +       atomic_set(&qmp->orphan, 1);
> This looks odd. Why are we letting the device be removed while it is in
> use by other drivers? Can't we pin the device with get_device() so it
> can't be removed and then prevent the driver from being removed until
> all the consumer drivers drop the reference, i.e. suppress sysfs unbind?
>
> Otherwise it looks like a generic problem that all provider devices,
> clks, regulators, gpios, etc. have to deal with and thus this
> hand-rolled mechanism can't be right.

As per my earlier discussion with Bjorn, device could be unbound using 
sysfs, in which case

remove() is called irrespective of whether any client driver is holding 
struct device reference

or not. That's why i have added separate refcount for qmp handle and 
marking it invalid if

qmp_remove() is called.

> >          mbox_free_channel(qmp->mbox_chan);
> > +       kref_put(&qmp->refcount, qmp_handle_release);
> >
> >          return 0;
> >   }