[PATCH v3 2/3] bus: mhi: host: Add name for mhi_controller

[PATCH v3 2/3] bus: mhi: host: Add name for mhi_controller

[Slark Xiao]

 For SDX72 MBIM mode, it starts data mux id from 112 instead of 0.
 This would lead to device can't ping outside successfully.
 Also MBIM side would report "bad packet session (112)".
 In oder to fix this issue, we decide to use the modem name
to do a match in client driver side. Then client driver could
set a corresponding mux_id value for this modem product.

Signed-off-by: Slark Xiao <slark_xiao@163.com>
---
 drivers/bus/mhi/host/pci_generic.c | 1 +
 include/linux/mhi.h                | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
index 1fb1c2f2fe12..14a11880bcea 100644
--- a/drivers/bus/mhi/host/pci_generic.c
+++ b/drivers/bus/mhi/host/pci_generic.c
@@ -1086,6 +1086,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	mhi_cntrl->runtime_get = mhi_pci_runtime_get;
 	mhi_cntrl->runtime_put = mhi_pci_runtime_put;
 	mhi_cntrl->mru = info->mru_default;
+	mhi_cntrl->name = info->name;
 
 	if (info->edl_trigger)
 		mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index b573f15762f8..86aa4f52842c 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -361,6 +361,7 @@ struct mhi_controller_config {
  * @wake_set: Device wakeup set flag
  * @irq_flags: irq flags passed to request_irq (optional)
  * @mru: the default MRU for the MHI device
+ * @name: name of the modem
  *
  * Fields marked as (required) need to be populated by the controller driver
  * before calling mhi_register_controller(). For the fields marked as (optional)
@@ -445,6 +446,7 @@ struct mhi_controller {
 	bool wake_set;
 	unsigned long irq_flags;
 	u32 mru;
+	const char *name;
 };
 
 /**
-- 
2.25.1

Re: [PATCH v3 2/3] bus: mhi: host: Add name for mhi_controller

[Jeffrey Hugo]

On 6/28/2024 1:36 AM, Slark Xiao wrote:
>   For SDX72 MBIM mode, it starts data mux id from 112 instead of 0.
>   This would lead to device can't ping outside successfully.
>   Also MBIM side would report "bad packet session (112)".

Weird indentation

>   In oder to fix this issue, we decide to use the modem name

"order"

> to do a match in client driver side. Then client driver could
> set a corresponding mux_id value for this modem product.
> 
> Signed-off-by: Slark Xiao <slark_xiao@163.com>
> ---
>   drivers/bus/mhi/host/pci_generic.c | 1 +
>   include/linux/mhi.h                | 2 ++
>   2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
> index 1fb1c2f2fe12..14a11880bcea 100644
> --- a/drivers/bus/mhi/host/pci_generic.c
> +++ b/drivers/bus/mhi/host/pci_generic.c
> @@ -1086,6 +1086,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   	mhi_cntrl->runtime_get = mhi_pci_runtime_get;
>   	mhi_cntrl->runtime_put = mhi_pci_runtime_put;
>   	mhi_cntrl->mru = info->mru_default;
> +	mhi_cntrl->name = info->name;
>   
>   	if (info->edl_trigger)
>   		mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index b573f15762f8..86aa4f52842c 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -361,6 +361,7 @@ struct mhi_controller_config {
>    * @wake_set: Device wakeup set flag
>    * @irq_flags: irq flags passed to request_irq (optional)
>    * @mru: the default MRU for the MHI device
> + * @name: name of the modem

Why restrict this to modems?  There are plenty of other MHI devices

>    *
>    * Fields marked as (required) need to be populated by the controller driver
>    * before calling mhi_register_controller(). For the fields marked as (optional)
> @@ -445,6 +446,7 @@ struct mhi_controller {
>   	bool wake_set;
>   	unsigned long irq_flags;
>   	u32 mru;
> +	const char *name;

Please run pahole

>   };
>   
>   /**

Re:Re: [PATCH v3 2/3] bus: mhi: host: Add name for mhi_controller

[Slark Xiao]

At 2024-06-28 22:38:57, "Jeffrey Hugo" <quic_jhugo@quicinc.com> wrote:
>On 6/28/2024 1:36 AM, Slark Xiao wrote:
>>   For SDX72 MBIM mode, it starts data mux id from 112 instead of 0.
>>   This would lead to device can't ping outside successfully.
>>   Also MBIM side would report "bad packet session (112)".
>

>Weird indentation

My mistake. Will be corrected in next.

>
>>   In oder to fix this issue, we decide to use the modem name
>
>"order"
>
>> to do a match in client driver side. Then client driver could
>> set a corresponding mux_id value for this modem product.
>> 
>> Signed-off-by: Slark Xiao <slark_xiao@163.com>
>> ---
>>   drivers/bus/mhi/host/pci_generic.c | 1 +
>>   include/linux/mhi.h                | 2 ++
>>   2 files changed, 3 insertions(+)
>> 
>> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
>> index 1fb1c2f2fe12..14a11880bcea 100644
>> --- a/drivers/bus/mhi/host/pci_generic.c
>> +++ b/drivers/bus/mhi/host/pci_generic.c
>> @@ -1086,6 +1086,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>   	mhi_cntrl->runtime_get = mhi_pci_runtime_get;
>>   	mhi_cntrl->runtime_put = mhi_pci_runtime_put;
>>   	mhi_cntrl->mru = info->mru_default;
>> +	mhi_cntrl->name = info->name;
>>   
>>   	if (info->edl_trigger)
>>   		mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
>> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
>> index b573f15762f8..86aa4f52842c 100644
>> --- a/include/linux/mhi.h
>> +++ b/include/linux/mhi.h
>> @@ -361,6 +361,7 @@ struct mhi_controller_config {
>>    * @wake_set: Device wakeup set flag
>>    * @irq_flags: irq flags passed to request_irq (optional)
>>    * @mru: the default MRU for the MHI device
>> + * @name: name of the modem
>

>Why restrict this to modems?  There are plenty of other MHI devices

Actually all MHI devices could be called modems. I don't think this is
a wrong name.

>
>>    *
>>    * Fields marked as (required) need to be populated by the controller driver
>>    * before calling mhi_register_controller(). For the fields marked as (optional)
>> @@ -445,6 +446,7 @@ struct mhi_controller {
>>   	bool wake_set;
>>   	unsigned long irq_flags;
>>   	u32 mru;
>> +	const char *name;
>

>Please run pahole

Emm, just checked,  there are 3 holes:
    u32                        M3;                   /*   312     4 */
    /* XXX 4 bytes hole, try to pack */
...
    bool                       wake_set;             /*   526     1 */
    /* XXX 1 byte hole, try to pack */
...
    u32                        mru;                  /*   536     4 */
    /* XXX 4 bytes hole, try to pack */

I will put 'const char *name' above 'u32 mru' to avoid the last hole.
Is this okay?

>
>>   };
>>   
>>   /**

Re: Re: [PATCH v3 2/3] bus: mhi: host: Add name for mhi_controller

[Manivannan Sadhasivam]

On Sat, Jun 29, 2024 at 04:03:28PM +0800, Slark Xiao wrote:
> 
> At 2024-06-28 22:38:57, "Jeffrey Hugo" <quic_jhugo@quicinc.com> wrote:
> >On 6/28/2024 1:36 AM, Slark Xiao wrote:
> >>   For SDX72 MBIM mode, it starts data mux id from 112 instead of 0.
> >>   This would lead to device can't ping outside successfully.
> >>   Also MBIM side would report "bad packet session (112)".
> >
> 
> >Weird indentation
> 
> My mistake. Will be corrected in next.
> 
> >
> >>   In oder to fix this issue, we decide to use the modem name
> >
> >"order"
> >
> >> to do a match in client driver side. Then client driver could
> >> set a corresponding mux_id value for this modem product.
> >> 
> >> Signed-off-by: Slark Xiao <slark_xiao@163.com>
> >> ---
> >>   drivers/bus/mhi/host/pci_generic.c | 1 +
> >>   include/linux/mhi.h                | 2 ++
> >>   2 files changed, 3 insertions(+)
> >> 
> >> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
> >> index 1fb1c2f2fe12..14a11880bcea 100644
> >> --- a/drivers/bus/mhi/host/pci_generic.c
> >> +++ b/drivers/bus/mhi/host/pci_generic.c
> >> @@ -1086,6 +1086,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >>   	mhi_cntrl->runtime_get = mhi_pci_runtime_get;
> >>   	mhi_cntrl->runtime_put = mhi_pci_runtime_put;
> >>   	mhi_cntrl->mru = info->mru_default;
> >> +	mhi_cntrl->name = info->name;
> >>   
> >>   	if (info->edl_trigger)
> >>   		mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
> >> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> >> index b573f15762f8..86aa4f52842c 100644
> >> --- a/include/linux/mhi.h
> >> +++ b/include/linux/mhi.h
> >> @@ -361,6 +361,7 @@ struct mhi_controller_config {
> >>    * @wake_set: Device wakeup set flag
> >>    * @irq_flags: irq flags passed to request_irq (optional)
> >>    * @mru: the default MRU for the MHI device
> >> + * @name: name of the modem
> >
> 
> >Why restrict this to modems?  There are plenty of other MHI devices
> 
> Actually all MHI devices could be called modems. I don't think this is
> a wrong name.
> 

No, not all MHI controllers are modems. This driver is a generic driver for MHI
controllers. So use below description:

	'Product or device name of the MHI controller'

> >
> >>    *
> >>    * Fields marked as (required) need to be populated by the controller driver
> >>    * before calling mhi_register_controller(). For the fields marked as (optional)
> >> @@ -445,6 +446,7 @@ struct mhi_controller {
> >>   	bool wake_set;
> >>   	unsigned long irq_flags;
> >>   	u32 mru;
> >> +	const char *name;
> >
> 
> >Please run pahole
> 
> Emm, just checked,  there are 3 holes:
>     u32                        M3;                   /*   312     4 */
>     /* XXX 4 bytes hole, try to pack */
> ...
>     bool                       wake_set;             /*   526     1 */
>     /* XXX 1 byte hole, try to pack */
> ...
>     u32                        mru;                  /*   536     4 */
>     /* XXX 4 bytes hole, try to pack */
> 
> I will put 'const char *name' above 'u32 mru' to avoid the last hole.
> Is this okay?
> 

Just put it at the top.

- Mani

-- 
மணிவண்ணன் சதாசிவம்