[PATCH 0/3] MHI patches for v5.16

[PATCH 0/3] MHI patches for v5.16

[Manivannan Sadhasivam]

Hi Greg,

Here is the small series of MHI patches for v5.16 with summary below:

1. MHI got a dedicated mailing list in subspace server. Hence, updated the
MAINTAINERS entry adding it as the primary list.

2. A leftover patch from 5.15 PR that adds a dedicated flag to the MHI client
transfer APIs for inbound buffer allocation by the MHI stack. This is required
for some downlink channels like QRTR that depends on pre-allocated buffers.
Since the patch modifies the MHI client drivers under "net/", Ack has been
collected from the netdev maintainer.

3. Fixed up the coccicheck warning by using sysfs_emit instead of snprintf.

Please consider merging!

Thanks,
Mani

Loic Poulain (1):
  bus: mhi: Add inbound buffers allocation flag

Manivannan Sadhasivam (1):
  MAINTAINERS: Update the entry for MHI bus

Qing Wang (1):
  bus: mhi: replace snprintf in show functions with sysfs_emit

 MAINTAINERS                      | 3 ++-
 drivers/bus/mhi/core/init.c      | 2 +-
 drivers/bus/mhi/core/internal.h  | 2 +-
 drivers/bus/mhi/core/main.c      | 9 ++++++---
 drivers/net/mhi_net.c            | 2 +-
 drivers/net/wwan/mhi_wwan_ctrl.c | 2 +-
 include/linux/mhi.h              | 7 ++++++-
 net/qrtr/mhi.c                   | 2 +-
 8 files changed, 19 insertions(+), 10 deletions(-)

-- 
2.25.1

[PATCH 1/3] MAINTAINERS: Update the entry for MHI bus

[Manivannan Sadhasivam]

Since Hemant is not carrying out any maintainership duties let's make
him as a dedicated reviewer. Also add the new mailing lists dedicated
for MHI in subspace mailing list server.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 MAINTAINERS | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index eeb4c70b3d5b..8ae357d746c1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12191,7 +12191,8 @@ F:	arch/arm64/boot/dts/marvell/armada-3720-uDPU.dts
 
 MHI BUS
 M:	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
-M:	Hemant Kumar <hemantk@codeaurora.org>
+R:	Hemant Kumar <hemantk@codeaurora.org>
+L:	mhi@lists.linux.dev
 L:	linux-arm-msm@vger.kernel.org
 S:	Maintained
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/mani/mhi.git
-- 
2.25.1

Re: [PATCH 1/3] MAINTAINERS: Update the entry for MHI bus

[Greg KH]

On Sat, Oct 16, 2021 at 12:27:32PM +0530, Manivannan Sadhasivam wrote:
> Since Hemant is not carrying out any maintainership duties let's make
> him as a dedicated reviewer. Also add the new mailing lists dedicated
> for MHI in subspace mailing list server.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  MAINTAINERS | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index eeb4c70b3d5b..8ae357d746c1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12191,7 +12191,8 @@ F:	arch/arm64/boot/dts/marvell/armada-3720-uDPU.dts
>  
>  MHI BUS
>  M:	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> -M:	Hemant Kumar <hemantk@codeaurora.org>
> +R:	Hemant Kumar <hemantk@codeaurora.org>
> +L:	mhi@lists.linux.dev

Can I get an acked-by from Hemant about this change please?

thanks,

greg k-h

Re: [PATCH 1/3] MAINTAINERS: Update the entry for MHI bus

[Hemant Kumar]

On 10/15/2021 11:57 PM, Manivannan Sadhasivam wrote:
> Since Hemant is not carrying out any maintainership duties let's make
> him as a dedicated reviewer. Also add the new mailing lists dedicated
> for MHI in subspace mailing list server.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, a Linux Foundation Collaborative Project

Re: [PATCH 1/3] MAINTAINERS: Update the entry for MHI bus

[Greg KH]

On Mon, Oct 18, 2021 at 09:19:16PM -0700, Hemant Kumar wrote:
> 
> 
> On 10/15/2021 11:57 PM, Manivannan Sadhasivam wrote:
> > Since Hemant is not carrying out any maintainership duties let's make
> > him as a dedicated reviewer. Also add the new mailing lists dedicated
> > for MHI in subspace mailing list server.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>

Great, Manivannan, can you resend just this patch with this reviewed-by
so that I can apply it?

thanks,

greg k-h

Re: [PATCH 1/3] MAINTAINERS: Update the entry for MHI bus

[Manivannan Sadhasivam]

On Tue, Oct 19, 2021 at 02:24:30PM +0200, Greg KH wrote:
> On Mon, Oct 18, 2021 at 09:19:16PM -0700, Hemant Kumar wrote:
> > 
> > 
> > On 10/15/2021 11:57 PM, Manivannan Sadhasivam wrote:
> > > Since Hemant is not carrying out any maintainership duties let's make
> > > him as a dedicated reviewer. Also add the new mailing lists dedicated
> > > for MHI in subspace mailing list server.
> > > 
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > 
> > Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>
> 
> Great, Manivannan, can you resend just this patch with this reviewed-by
> so that I can apply it?
> 

Sure, will do. I thought I could also send the modified version of patch 2 of
this series but we ran out of time :/

Thanks,
Mani

> thanks,
> 
> greg k-h

[PATCH 2/3] bus: mhi: Add inbound buffers allocation flag

[Manivannan Sadhasivam]

From: Loic Poulain <loic.poulain@linaro.org>

Currently, the MHI controller driver defines which channels should
have their inbound buffers allocated and queued. But ideally, this is
something that should be decided by the MHI device driver instead,
which actually deals with that buffers.

Add a flag parameter to mhi_prepare_for_transfer allowing to specify
if buffers have to be allocated and queued by the MHI stack.

Keep auto_queue flag for now, but should be removed at some point.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
Tested-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
Reviewed-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Acked-by: Jakub Kicinski <kuba@kernel.org>
Link: https://lore.kernel.org/r/1624566520-20406-1-git-send-email-loic.poulain@linaro.org
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/bus/mhi/core/internal.h  | 2 +-
 drivers/bus/mhi/core/main.c      | 9 ++++++---
 drivers/net/mhi_net.c            | 2 +-
 drivers/net/wwan/mhi_wwan_ctrl.c | 2 +-
 include/linux/mhi.h              | 7 ++++++-
 net/qrtr/mhi.c                   | 2 +-
 6 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
index 3a732afaf73e..721739c5e0d5 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -682,7 +682,7 @@ void mhi_rddm_prepare(struct mhi_controller *mhi_cntrl,
 		      struct image_info *img_info);
 void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl);
 int mhi_prepare_channel(struct mhi_controller *mhi_cntrl,
-			struct mhi_chan *mhi_chan);
+			struct mhi_chan *mhi_chan, unsigned int flags);
 int mhi_init_chan_ctxt(struct mhi_controller *mhi_cntrl,
 		       struct mhi_chan *mhi_chan);
 void mhi_deinit_chan_ctxt(struct mhi_controller *mhi_cntrl,
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index b15c5bc37dd4..c01ec2fef02c 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -1430,7 +1430,7 @@ static void mhi_unprepare_channel(struct mhi_controller *mhi_cntrl,
 }
 
 int mhi_prepare_channel(struct mhi_controller *mhi_cntrl,
-			struct mhi_chan *mhi_chan)
+			struct mhi_chan *mhi_chan, unsigned int flags)
 {
 	int ret = 0;
 	struct device *dev = &mhi_chan->mhi_dev->dev;
@@ -1455,6 +1455,9 @@ int mhi_prepare_channel(struct mhi_controller *mhi_cntrl,
 	if (ret)
 		goto error_pm_state;
 
+	if (mhi_chan->dir == DMA_FROM_DEVICE)
+		mhi_chan->pre_alloc = !!(flags & MHI_CH_INBOUND_ALLOC_BUFS);
+
 	/* Pre-allocate buffer for xfer ring */
 	if (mhi_chan->pre_alloc) {
 		int nr_el = get_nr_avail_ring_elements(mhi_cntrl,
@@ -1610,7 +1613,7 @@ void mhi_reset_chan(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan)
 }
 
 /* Move channel to start state */
-int mhi_prepare_for_transfer(struct mhi_device *mhi_dev)
+int mhi_prepare_for_transfer(struct mhi_device *mhi_dev, unsigned int flags)
 {
 	int ret, dir;
 	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
@@ -1621,7 +1624,7 @@ int mhi_prepare_for_transfer(struct mhi_device *mhi_dev)
 		if (!mhi_chan)
 			continue;
 
-		ret = mhi_prepare_channel(mhi_cntrl, mhi_chan);
+		ret = mhi_prepare_channel(mhi_cntrl, mhi_chan, flags);
 		if (ret)
 			goto error_open_chan;
 	}
diff --git a/drivers/net/mhi_net.c b/drivers/net/mhi_net.c
index d127eb6e9257..975f7f9bdf4c 100644
--- a/drivers/net/mhi_net.c
+++ b/drivers/net/mhi_net.c
@@ -319,7 +319,7 @@ static int mhi_net_newlink(struct mhi_device *mhi_dev, struct net_device *ndev)
 	u64_stats_init(&mhi_netdev->stats.tx_syncp);
 
 	/* Start MHI channels */
-	err = mhi_prepare_for_transfer(mhi_dev);
+	err = mhi_prepare_for_transfer(mhi_dev, 0);
 	if (err)
 		goto out_err;
 
diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
index e4d0f696687f..d0a98f34c54d 100644
--- a/drivers/net/wwan/mhi_wwan_ctrl.c
+++ b/drivers/net/wwan/mhi_wwan_ctrl.c
@@ -110,7 +110,7 @@ static int mhi_wwan_ctrl_start(struct wwan_port *port)
 	int ret;
 
 	/* Start mhi device's channel(s) */
-	ret = mhi_prepare_for_transfer(mhiwwan->mhi_dev);
+	ret = mhi_prepare_for_transfer(mhiwwan->mhi_dev, 0);
 	if (ret)
 		return ret;
 
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index 723985879035..56e7934f8b23 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -723,8 +723,13 @@ void mhi_device_put(struct mhi_device *mhi_dev);
  *                            host and device execution environments match and
  *                            channels are in a DISABLED state.
  * @mhi_dev: Device associated with the channels
+ * @flags: MHI channel flags
  */
-int mhi_prepare_for_transfer(struct mhi_device *mhi_dev);
+int mhi_prepare_for_transfer(struct mhi_device *mhi_dev,
+			     unsigned int flags);
+
+/* Automatically allocate and queue inbound buffers */
+#define MHI_CH_INBOUND_ALLOC_BUFS BIT(0)
 
 /**
  * mhi_unprepare_from_transfer - Reset UL and DL channels for data transfer.
diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c
index fa611678af05..29b4fa3b72ab 100644
--- a/net/qrtr/mhi.c
+++ b/net/qrtr/mhi.c
@@ -79,7 +79,7 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev,
 	int rc;
 
 	/* start channels */
-	rc = mhi_prepare_for_transfer(mhi_dev);
+	rc = mhi_prepare_for_transfer(mhi_dev, MHI_CH_INBOUND_ALLOC_BUFS);
 	if (rc)
 		return rc;
 
-- 
2.25.1

Re: [PATCH 2/3] bus: mhi: Add inbound buffers allocation flag

[Greg KH]

On Sat, Oct 16, 2021 at 12:27:33PM +0530, Manivannan Sadhasivam wrote:
> From: Loic Poulain <loic.poulain@linaro.org>
> 
> Currently, the MHI controller driver defines which channels should
> have their inbound buffers allocated and queued. But ideally, this is
> something that should be decided by the MHI device driver instead,
> which actually deals with that buffers.
> 
> Add a flag parameter to mhi_prepare_for_transfer allowing to specify
> if buffers have to be allocated and queued by the MHI stack.

This is a horrible api.  Now one has to go and look up why "0" was added
to a function as a parameter.

If you don't want to allocate the buffer, then make a function of that
name and call that.  As you only have one "flag", don't try to make
something generic here that is obviously not generic at all.

You all can do better than this.

thanks,

greg k-h

Re: [PATCH 2/3] bus: mhi: Add inbound buffers allocation flag

[Manivannan Sadhasivam]

On Sat, Oct 16, 2021 at 09:39:53AM +0200, Greg KH wrote:
> On Sat, Oct 16, 2021 at 12:27:33PM +0530, Manivannan Sadhasivam wrote:
> > From: Loic Poulain <loic.poulain@linaro.org>
> > 
> > Currently, the MHI controller driver defines which channels should
> > have their inbound buffers allocated and queued. But ideally, this is
> > something that should be decided by the MHI device driver instead,
> > which actually deals with that buffers.
> > 
> > Add a flag parameter to mhi_prepare_for_transfer allowing to specify
> > if buffers have to be allocated and queued by the MHI stack.
> 
> This is a horrible api.  Now one has to go and look up why "0" was added
> to a function as a parameter.
> 
> If you don't want to allocate the buffer, then make a function of that
> name and call that.  As you only have one "flag", don't try to make
> something generic here that is obviously not generic at all.
> 

This is the only API that can be used by the client drivers to pass the
configurations to the MHI stack. So we wanted to have a flags parameter that
could be extended in the future also.

Regarding "0", the default behaviour is to not pre allocate the buffer at all.
So it made less sense to add a separate flag or an API for that.

Thanks,
Mani

> You all can do better than this.
> 
> thanks,
> 
> greg k-h

Re: [PATCH 2/3] bus: mhi: Add inbound buffers allocation flag

[Greg KH]

On Sat, Oct 16, 2021 at 10:01:28PM +0530, Manivannan Sadhasivam wrote:
> On Sat, Oct 16, 2021 at 09:39:53AM +0200, Greg KH wrote:
> > On Sat, Oct 16, 2021 at 12:27:33PM +0530, Manivannan Sadhasivam wrote:
> > > From: Loic Poulain <loic.poulain@linaro.org>
> > > 
> > > Currently, the MHI controller driver defines which channels should
> > > have their inbound buffers allocated and queued. But ideally, this is
> > > something that should be decided by the MHI device driver instead,
> > > which actually deals with that buffers.
> > > 
> > > Add a flag parameter to mhi_prepare_for_transfer allowing to specify
> > > if buffers have to be allocated and queued by the MHI stack.
> > 
> > This is a horrible api.  Now one has to go and look up why "0" was added
> > to a function as a parameter.
> > 
> > If you don't want to allocate the buffer, then make a function of that
> > name and call that.  As you only have one "flag", don't try to make
> > something generic here that is obviously not generic at all.
> > 
> 
> This is the only API that can be used by the client drivers to pass the
> configurations to the MHI stack. So we wanted to have a flags parameter that
> could be extended in the future also.

Worry about future issues then, in the future :)

> Regarding "0", the default behaviour is to not pre allocate the buffer at all.
> So it made less sense to add a separate flag or an API for that.

But again, this is now hard to understand and if you run across a '0' in
the call, you have to go and look it up, breaking your reading flow.

Please just create a new function for this new option, and then have
both of them call the common function with a boolean for this "allocate
or not" type of thing.  We do this all the time in the kernel to make it
easier to read and understand over time.

thanks,

greg k-h

[PATCH 3/3] bus: mhi: replace snprintf in show functions with sysfs_emit

[Manivannan Sadhasivam]

From: Qing Wang <wangqing@vivo.com>

coccicheck complains about the use of snprintf() in sysfs show functions.

Fix the following coccicheck warning:
drivers/bus/mhi/core/init.c:97:8-16: WARNING: use scnprintf or sprintf.

Use sysfs_emit instead of scnprintf or sprintf makes more sense.

Signed-off-by: Qing Wang <wangqing@vivo.com>
Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Link: https://lore.kernel.org/r/1634095550-3978-1-git-send-email-wangqing@vivo.com
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/bus/mhi/core/init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index 5aaca6d0f52b..a5a5c722731e 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -94,7 +94,7 @@ static ssize_t serial_number_show(struct device *dev,
 	struct mhi_device *mhi_dev = to_mhi_device(dev);
 	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
 
-	return snprintf(buf, PAGE_SIZE, "Serial Number: %u\n",
+	return sysfs_emit(buf, "Serial Number: %u\n",
 			mhi_cntrl->serial_number);
 }
 static DEVICE_ATTR_RO(serial_number);
-- 
2.25.1

Re: [PATCH 3/3] bus: mhi: replace snprintf in show functions with sysfs_emit

[Greg KH]

On Sat, Oct 16, 2021 at 12:27:34PM +0530, Manivannan Sadhasivam wrote:
> From: Qing Wang <wangqing@vivo.com>
> 
> coccicheck complains about the use of snprintf() in sysfs show functions.
> 
> Fix the following coccicheck warning:
> drivers/bus/mhi/core/init.c:97:8-16: WARNING: use scnprintf or sprintf.
> 
> Use sysfs_emit instead of scnprintf or sprintf makes more sense.
> 
> Signed-off-by: Qing Wang <wangqing@vivo.com>
> Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>
> Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
> Link: https://lore.kernel.org/r/1634095550-3978-1-git-send-email-wangqing@vivo.com
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/bus/mhi/core/init.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index 5aaca6d0f52b..a5a5c722731e 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -94,7 +94,7 @@ static ssize_t serial_number_show(struct device *dev,
>  	struct mhi_device *mhi_dev = to_mhi_device(dev);
>  	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
>  
> -	return snprintf(buf, PAGE_SIZE, "Serial Number: %u\n",
> +	return sysfs_emit(buf, "Serial Number: %u\n",
>  			mhi_cntrl->serial_number);

The text "Serial Number: " should not be in here, right?  It's obvious
this is a serial number, that's what the documentation and file name
says.  Userspace should not have to parse sysfs files.

And why is only one sysfs entry being changed in this file?  Either they
all should be, or none, no need to do this one-patch-per-entry, right?

Note, I have rejected Qing's patches like this for other subsystems
already because they are not complete, this is something they are well
aware of by now...

thanks,

greg k-h

Re: [PATCH 3/3] bus: mhi: replace snprintf in show functions with sysfs_emit

[Joe Perches]

On Sat, 2021-10-16 at 09:37 +0200, Greg KH wrote:
> On Sat, Oct 16, 2021 at 12:27:34PM +0530, Manivannan Sadhasivam wrote:
> > From: Qing Wang <wangqing@vivo.com>
> > coccicheck complains about the use of snprintf() in sysfs show functions.
[]
> > diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
[]
> > @@ -94,7 +94,7 @@ static ssize_t serial_number_show(struct device *dev,
> >  	struct mhi_device *mhi_dev = to_mhi_device(dev);
> >  	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> >  
> > -	return snprintf(buf, PAGE_SIZE, "Serial Number: %u\n",
> > +	return sysfs_emit(buf, "Serial Number: %u\n",
> >  			mhi_cntrl->serial_number);
> 
> The text "Serial Number: " should not be in here, right?  It's obvious
> this is a serial number, that's what the documentation and file name
> says.  Userspace should not have to parse sysfs files.

sysfs is ABI right?  Parsing or not, it's what's already there.

Re: [PATCH 3/3] bus: mhi: replace snprintf in show functions with sysfs_emit

[Greg KH]

On Sat, Oct 16, 2021 at 03:24:17AM -0700, Joe Perches wrote:
> On Sat, 2021-10-16 at 09:37 +0200, Greg KH wrote:
> > On Sat, Oct 16, 2021 at 12:27:34PM +0530, Manivannan Sadhasivam wrote:
> > > From: Qing Wang <wangqing@vivo.com>
> > > coccicheck complains about the use of snprintf() in sysfs show functions.
> []
> > > diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> []
> > > @@ -94,7 +94,7 @@ static ssize_t serial_number_show(struct device *dev,
> > >  	struct mhi_device *mhi_dev = to_mhi_device(dev);
> > >  	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> > >  
> > > -	return snprintf(buf, PAGE_SIZE, "Serial Number: %u\n",
> > > +	return sysfs_emit(buf, "Serial Number: %u\n",
> > >  			mhi_cntrl->serial_number);
> > 
> > The text "Serial Number: " should not be in here, right?  It's obvious
> > this is a serial number, that's what the documentation and file name
> > says.  Userspace should not have to parse sysfs files.
> 
> sysfs is ABI right?  Parsing or not, it's what's already there.

If no tools rely on this, and we can change it, we should at least try.

We can not change ABI if something breaks.  If nothing relies on it,
then it is fine to do so.

thanks,

greg k-h

Re: [PATCH 3/3] bus: mhi: replace snprintf in show functions with sysfs_emit

[Joe Perches]

On Sat, 2021-10-16 at 17:07 +0200, Greg KH wrote:
> On Sat, Oct 16, 2021 at 03:24:17AM -0700, Joe Perches wrote:
> > On Sat, 2021-10-16 at 09:37 +0200, Greg KH wrote:
> > > On Sat, Oct 16, 2021 at 12:27:34PM +0530, Manivannan Sadhasivam wrote:
> > > > From: Qing Wang <wangqing@vivo.com>
> > > > coccicheck complains about the use of snprintf() in sysfs show functions.
> > []
> > > > diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> > []
> > > > @@ -94,7 +94,7 @@ static ssize_t serial_number_show(struct device *dev,
> > > >  	struct mhi_device *mhi_dev = to_mhi_device(dev);
> > > >  	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> > > >  
> > > > -	return snprintf(buf, PAGE_SIZE, "Serial Number: %u\n",
> > > > +	return sysfs_emit(buf, "Serial Number: %u\n",
> > > >  			mhi_cntrl->serial_number);
> > > 
> > > The text "Serial Number: " should not be in here, right?  It's obvious
> > > this is a serial number, that's what the documentation and file name
> > > says.  Userspace should not have to parse sysfs files.
> > 
> > sysfs is ABI right?  Parsing or not, it's what's already there.
> 
> If no tools rely on this, and we can change it, we should at least try.
> 
> We can not change ABI if something breaks.  If nothing relies on it,
> then it is fine to do so.

That's a quite bad way to think of an ABI.

All that does is tempt fate as you don't know if something already
uses it until someone complains and by that time something else may
be written to depend on the new behavior.

Re: [PATCH 3/3] bus: mhi: replace snprintf in show functions with sysfs_emit

[Manivannan Sadhasivam]

On Sat, Oct 16, 2021 at 05:07:06PM +0200, Greg KH wrote:
> On Sat, Oct 16, 2021 at 03:24:17AM -0700, Joe Perches wrote:
> > On Sat, 2021-10-16 at 09:37 +0200, Greg KH wrote:
> > > On Sat, Oct 16, 2021 at 12:27:34PM +0530, Manivannan Sadhasivam wrote:
> > > > From: Qing Wang <wangqing@vivo.com>
> > > > coccicheck complains about the use of snprintf() in sysfs show functions.
> > []
> > > > diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> > []
> > > > @@ -94,7 +94,7 @@ static ssize_t serial_number_show(struct device *dev,
> > > >  	struct mhi_device *mhi_dev = to_mhi_device(dev);
> > > >  	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> > > >  
> > > > -	return snprintf(buf, PAGE_SIZE, "Serial Number: %u\n",
> > > > +	return sysfs_emit(buf, "Serial Number: %u\n",
> > > >  			mhi_cntrl->serial_number);
> > > 
> > > The text "Serial Number: " should not be in here, right?  It's obvious
> > > this is a serial number, that's what the documentation and file name
> > > says.  Userspace should not have to parse sysfs files.
> > 
> > sysfs is ABI right?  Parsing or not, it's what's already there.
> 
> If no tools rely on this, and we can change it, we should at least try.
> 
> We can not change ABI if something breaks.  If nothing relies on it,
> then it is fine to do so.
> 

Hemant, Bhaumik, do you guys know if there are any possible users (scripts/apps)
of this ABI? I'm not 100% inclined to change it but if we are _sure_ that there
are no users yet, then I'm ok with it.

Thanks,
Mani

> thanks,
> 
> greg k-h

Re: [PATCH 3/3] bus: mhi: replace snprintf in show functions with sysfs_emit

[Hemant Kumar]

On 10/16/2021 9:15 AM, Manivannan Sadhasivam wrote:
> On Sat, Oct 16, 2021 at 05:07:06PM +0200, Greg KH wrote:
>> On Sat, Oct 16, 2021 at 03:24:17AM -0700, Joe Perches wrote:
>>> On Sat, 2021-10-16 at 09:37 +0200, Greg KH wrote:
>>>> On Sat, Oct 16, 2021 at 12:27:34PM +0530, Manivannan Sadhasivam wrote:
>>>>> From: Qing Wang <wangqing@vivo.com>
>>>>> coccicheck complains about the use of snprintf() in sysfs show functions.
>>> []
>>>>> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
>>> []
>>>>> @@ -94,7 +94,7 @@ static ssize_t serial_number_show(struct device *dev,
>>>>>   	struct mhi_device *mhi_dev = to_mhi_device(dev);
>>>>>   	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
>>>>>   
>>>>> -	return snprintf(buf, PAGE_SIZE, "Serial Number: %u\n",
>>>>> +	return sysfs_emit(buf, "Serial Number: %u\n",
>>>>>   			mhi_cntrl->serial_number);
>>>>
>>>> The text "Serial Number: " should not be in here, right?  It's obvious
>>>> this is a serial number, that's what the documentation and file name
>>>> says.  Userspace should not have to parse sysfs files.
>>>
>>> sysfs is ABI right?  Parsing or not, it's what's already there.
>>
>> If no tools rely on this, and we can change it, we should at least try.
>>
>> We can not change ABI if something breaks.  If nothing relies on it,
>> then it is fine to do so.
>>
> 
> Hemant, Bhaumik, do you guys know if there are any possible users (scripts/apps)
> of this ABI? I'm not 100% inclined to change it but if we are _sure_ that there
> are no users yet, then I'm ok with it.

Mani, i dont know if any script being used by any user to parse "Serial 
Number".

Thanks,
Hemant
> 
> Thanks,
> Mani
> 
>> thanks,
>>
>> greg k-h

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, a Linux Foundation Collaborative Project

Re: [PATCH 3/3] bus: mhi: replace snprintf in show functions with sysfs_emit

[Manivannan Sadhasivam]

On Sat, Oct 16, 2021 at 09:37:50AM +0200, Greg KH wrote:
> On Sat, Oct 16, 2021 at 12:27:34PM +0530, Manivannan Sadhasivam wrote:
> > From: Qing Wang <wangqing@vivo.com>
> > 
> > coccicheck complains about the use of snprintf() in sysfs show functions.
> > 
> > Fix the following coccicheck warning:
> > drivers/bus/mhi/core/init.c:97:8-16: WARNING: use scnprintf or sprintf.
> > 
> > Use sysfs_emit instead of scnprintf or sprintf makes more sense.
> > 
> > Signed-off-by: Qing Wang <wangqing@vivo.com>
> > Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>
> > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
> > Link: https://lore.kernel.org/r/1634095550-3978-1-git-send-email-wangqing@vivo.com
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/bus/mhi/core/init.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> > index 5aaca6d0f52b..a5a5c722731e 100644
> > --- a/drivers/bus/mhi/core/init.c
> > +++ b/drivers/bus/mhi/core/init.c
> > @@ -94,7 +94,7 @@ static ssize_t serial_number_show(struct device *dev,
> >  	struct mhi_device *mhi_dev = to_mhi_device(dev);
> >  	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> >  
> > -	return snprintf(buf, PAGE_SIZE, "Serial Number: %u\n",
> > +	return sysfs_emit(buf, "Serial Number: %u\n",
> >  			mhi_cntrl->serial_number);
> 
> The text "Serial Number: " should not be in here, right?  It's obvious
> this is a serial number, that's what the documentation and file name
> says.  Userspace should not have to parse sysfs files.
> 

Right, somehow missed it :/

> And why is only one sysfs entry being changed in this file?  Either they
> all should be, or none, no need to do this one-patch-per-entry, right?
> 
> Note, I have rejected Qing's patches like this for other subsystems
> already because they are not complete, this is something they are well
> aware of by now...
> 

Oh, I'm not aware of this.

Qing: Please modify the other instance of snprintf also.

Thanks,
Mani

> thanks,
> 
> greg k-h

Re: [PATCH 0/3] MHI patches for v5.16

[Manivannan Sadhasivam]

On Sat, Oct 16, 2021 at 12:27:31PM +0530, Manivannan Sadhasivam wrote:
> Hi Greg,
> 
> Here is the small series of MHI patches for v5.16 with summary below:
> 
> 1. MHI got a dedicated mailing list in subspace server. Hence, updated the
> MAINTAINERS entry adding it as the primary list.
> 

Missed to mention that now Hemant is a dedicated reviewer since he is not
handling any of the maintainership duties.

Thanks,
Mani

> 2. A leftover patch from 5.15 PR that adds a dedicated flag to the MHI client
> transfer APIs for inbound buffer allocation by the MHI stack. This is required
> for some downlink channels like QRTR that depends on pre-allocated buffers.
> Since the patch modifies the MHI client drivers under "net/", Ack has been
> collected from the netdev maintainer.
> 
> 3. Fixed up the coccicheck warning by using sysfs_emit instead of snprintf.
> 
> Please consider merging!
> 
> Thanks,
> Mani
> 
> Loic Poulain (1):
>   bus: mhi: Add inbound buffers allocation flag
> 
> Manivannan Sadhasivam (1):
>   MAINTAINERS: Update the entry for MHI bus
> 
> Qing Wang (1):
>   bus: mhi: replace snprintf in show functions with sysfs_emit
> 
>  MAINTAINERS                      | 3 ++-
>  drivers/bus/mhi/core/init.c      | 2 +-
>  drivers/bus/mhi/core/internal.h  | 2 +-
>  drivers/bus/mhi/core/main.c      | 9 ++++++---
>  drivers/net/mhi_net.c            | 2 +-
>  drivers/net/wwan/mhi_wwan_ctrl.c | 2 +-
>  include/linux/mhi.h              | 7 ++++++-
>  net/qrtr/mhi.c                   | 2 +-
>  8 files changed, 19 insertions(+), 10 deletions(-)
> 
> -- 
> 2.25.1
>