[PATCH v2 2/2] net: wwan: mhi: make default data link id configurable

[PATCH v2 2/2] net: wwan: mhi: make default data link id configurable

[Slark Xiao]

For SDX72 MBIM device, 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)".
So we add a link id default value for these SDX72 products which
works in MBIM mode.

Signed-off-by: Slark Xiao <slark_xiao@163.com>
---
 drivers/net/wwan/mhi_wwan_mbim.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wwan/mhi_wwan_mbim.c b/drivers/net/wwan/mhi_wwan_mbim.c
index 3f72ae943b29..c731fe20814f 100644
--- a/drivers/net/wwan/mhi_wwan_mbim.c
+++ b/drivers/net/wwan/mhi_wwan_mbim.c
@@ -618,7 +618,8 @@ static int mhi_mbim_probe(struct mhi_device *mhi_dev, const struct mhi_device_id
 	mbim->rx_queue_sz = mhi_get_free_desc_count(mhi_dev, DMA_FROM_DEVICE);
 
 	/* Register wwan link ops with MHI controller representing WWAN instance */
-	return wwan_register_ops(&cntrl->mhi_dev->dev, &mhi_mbim_wwan_ops, mbim, 0);
+	return wwan_register_ops(&cntrl->mhi_dev->dev, &mhi_mbim_wwan_ops, mbim,
+		mhi_dev->mhi_cntrl->link_id);
 }
 
 static void mhi_mbim_remove(struct mhi_device *mhi_dev)
-- 
2.25.1

Re: [PATCH v2 2/2] net: wwan: mhi: make default data link id configurable

[Sergey Ryazanov]

Hello Slark, Manivannan,

On 12.06.2024 12:39, Slark Xiao wrote:
> For SDX72 MBIM device, 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)".
> So we add a link id default value for these SDX72 products which
> works in MBIM mode.

The patch itself looks good to me except a tiny nitpick (see below). 
Meanwhile, I can not understand when we should merge it. During the V1 
discussion, It was mentioned that we need this change specifically for 
Foxconn SDX72 modem. Without any actual users the configurable default 
data link id is a dead code.

According to the ARM MSM patchwork [1], the main Foxconn SDX72 
introducing patch is (a) not yet merged, (b) no more applicable. So, as 
far as I understand, it should be resend. In this context, a best way to 
merge the modem support is to prepend the modem introduction patch with 
these changes forming a series:
1/3: bus: mhi: host: Import mux_id item
2/3: net: wwan: mhi: make default data link id configurable
3/3: bus: mhi: host: Add Foxconn SDX72 related support

And merge the series as whole, when everything will be ready. This will 
help us to avoid partially merged work and will keep the modem support 
introduction clear.

Manivannan, could you share the main [1] Foxconn SDX72 introduction 
patch status, and your thoughts regarding the merging process?


1. 
https://patchwork.kernel.org/project/linux-arm-msm/patch/20240520070633.308913-1-slark_xiao@163.com/

> Signed-off-by: Slark Xiao <slark_xiao@163.com>
> ---
>   drivers/net/wwan/mhi_wwan_mbim.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wwan/mhi_wwan_mbim.c b/drivers/net/wwan/mhi_wwan_mbim.c
> index 3f72ae943b29..c731fe20814f 100644
> --- a/drivers/net/wwan/mhi_wwan_mbim.c
> +++ b/drivers/net/wwan/mhi_wwan_mbim.c
> @@ -618,7 +618,8 @@ static int mhi_mbim_probe(struct mhi_device *mhi_dev, const struct mhi_device_id
>   	mbim->rx_queue_sz = mhi_get_free_desc_count(mhi_dev, DMA_FROM_DEVICE);
>   
>   	/* Register wwan link ops with MHI controller representing WWAN instance */
> -	return wwan_register_ops(&cntrl->mhi_dev->dev, &mhi_mbim_wwan_ops, mbim, 0);
> +	return wwan_register_ops(&cntrl->mhi_dev->dev, &mhi_mbim_wwan_ops, mbim,
> +		mhi_dev->mhi_cntrl->link_id);

Just a nitpick. The second line had better be aligned with the opening 
bracket:

return wwan_register_ops(&cntrl->...
                          mhi_dev->...

>   }
>   
>   static void mhi_mbim_remove(struct mhi_device *mhi_dev)

--
Sergey

Re:Re: [PATCH v2 2/2] net: wwan: mhi: make default data link id configurable

[Slark Xiao]

At 2024-06-13 05:54:03, "Sergey Ryazanov" <ryazanov.s.a@gmail.com> wrote:
>Hello Slark, Manivannan,
>
>On 12.06.2024 12:39, Slark Xiao wrote:
>> For SDX72 MBIM device, 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)".
>> So we add a link id default value for these SDX72 products which
>> works in MBIM mode.
>
>The patch itself looks good to me except a tiny nitpick (see below). 
>Meanwhile, I can not understand when we should merge it. During the V1 
>discussion, It was mentioned that we need this change specifically for 
>Foxconn SDX72 modem. Without any actual users the configurable default 
>data link id is a dead code.
>
>According to the ARM MSM patchwork [1], the main Foxconn SDX72 
>introducing patch is (a) not yet merged, (b) no more applicable. So, as 
>far as I understand, it should be resend. In this context, a best way to 
>merge the modem support is to prepend the modem introduction patch with 
>these changes forming a series:
>1/3: bus: mhi: host: Import mux_id item
>2/3: net: wwan: mhi: make default data link id configurable
>3/3: bus: mhi: host: Add Foxconn SDX72 related support
>
>And merge the series as whole, when everything will be ready. This will 
>help us to avoid partially merged work and will keep the modem support 
>introduction clear.
>

Yes, currently these 3 patches would be merged by Mani at the same time.
So I think there is no build failure risk.

>Manivannan, could you share the main [1] Foxconn SDX72 introduction 
>patch status, and your thoughts regarding the merging process?

We were discussing another patch in last weeks. And we still have not
reached a consensus. Let's focus on that patch firstly.
And Mani, please let us know about the merging process since the new
merge window is open or will open soon?

Thanks
>
>
>1. 
>https://patchwork.kernel.org/project/linux-arm-msm/patch/20240520070633.308913-1-slark_xiao@163.com/
>
>> Signed-off-by: Slark Xiao <slark_xiao@163.com>
>> ---
>>   drivers/net/wwan/mhi_wwan_mbim.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/wwan/mhi_wwan_mbim.c b/drivers/net/wwan/mhi_wwan_mbim.c
>> index 3f72ae943b29..c731fe20814f 100644
>> --- a/drivers/net/wwan/mhi_wwan_mbim.c
>> +++ b/drivers/net/wwan/mhi_wwan_mbim.c
>> @@ -618,7 +618,8 @@ static int mhi_mbim_probe(struct mhi_device *mhi_dev, const struct mhi_device_id
>>   	mbim->rx_queue_sz = mhi_get_free_desc_count(mhi_dev, DMA_FROM_DEVICE);
>>   
>>   	/* Register wwan link ops with MHI controller representing WWAN instance */
>> -	return wwan_register_ops(&cntrl->mhi_dev->dev, &mhi_mbim_wwan_ops, mbim, 0);
>> +	return wwan_register_ops(&cntrl->mhi_dev->dev, &mhi_mbim_wwan_ops, mbim,
>> +		mhi_dev->mhi_cntrl->link_id);
>
>Just a nitpick. The second line had better be aligned with the opening 
>bracket:
>
>return wwan_register_ops(&cntrl->...
>                          mhi_dev->...
>
>>   }
>>   
>>   static void mhi_mbim_remove(struct mhi_device *mhi_dev)
>
>--
>Sergey