Re:Re: [PATCH v1 2/2] net: wwan: Fix SDX72 ping failure issue

Re:Re: [PATCH v1 2/2] net: wwan: Fix SDX72 ping failure issue

[Slark Xiao]

+More maintainer to this second patch list.

At 2024-06-08 06:28:48, "Sergey Ryazanov" <ryazanov.s.a@gmail.com> wrote:
>Hello Slark,
>
>without the first patch it is close to impossible to understand this 
>one. Next time please send such tightly connected patches to both 
>mailing lists.
>
Sorry for this mistake since it's my first commit about committing code to 2
difference area: mhi and mbim. Both the maintainers are difference.
In case a new version commit would be created, I would like to ask if
should I add both side maintainers on these 2 patches ?
 
>On 07.06.2024 13:03, 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.
>> 
>> Signed-off-by: Slark Xiao <slark_xiao@163.com>
>
>Since it a but fix, it needs a 'Fixes:' tag.
>
Actually, I thought it's a fix for common SDX72 product. But now I think
it should be only meet for my SDX72 MBIM product. Previous commit 
has not been applied. So there is no commit id for "Fixes".
But I think I shall include that patch in V2 version.
Please ref: 
https://lore.kernel.org/lkml/20240520070633.308913-1-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..4ca5c845394b 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 ? mhi_dev->mhi_cntrl->link_id : 0);
>
>Is it possible to drop the ternary operator and pass the link_id directly?
>
>>   }
>>   
>>   static void mhi_mbim_remove(struct mhi_device *mhi_dev)

Re: [PATCH v1 2/2] net: wwan: Fix SDX72 ping failure issue

[Sergey Ryazanov]

On 11.06.2024 04:36, Slark Xiao wrote:
> +More maintainer to this second patch list.
> 
> At 2024-06-08 06:28:48, "Sergey Ryazanov" <ryazanov.s.a@gmail.com> wrote:
>> Hello Slark,
>>
>> without the first patch it is close to impossible to understand this
>> one. Next time please send such tightly connected patches to both
>> mailing lists.
>>
> Sorry for this mistake since it's my first commit about committing code to 2
> difference area: mhi and mbim. Both the maintainers are difference.
> In case a new version commit would be created, I would like to ask if
> should I add both side maintainers on these 2 patches ?

No worries. We finally got both sides of the puzzle. BTW, looks like the 
first patch still lacks Linux netdev mailing list in the CC.

Usually maintainers are responsible for applying patches to their 
dedicated repositories (trees), and then eventually for sending them in 
batch to the main tree. So, if a work consists of two patches, it is 
better to apply them together to one of the trees. Otherwise, it can 
cause a build failure in one tree due to lack of required changes that 
have been applied to other. Sometimes contributors even specify a 
preferred tree in a cover letter. However, it is still up to maintainers 
to make a decision which tree is better when a work changes several 
subsystems.

>> On 07.06.2024 13:03, 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.
>>>
>>> Signed-off-by: Slark Xiao <slark_xiao@163.com>
>>
>> Since it a but fix, it needs a 'Fixes:' tag.
>>
> Actually, I thought it's a fix for common SDX72 product. But now I think
> it should be only meet for my SDX72 MBIM product. Previous commit
> has not been applied. So there is no commit id for "Fixes".
> But I think I shall include that patch in V2 version.
> Please ref:
> https://lore.kernel.org/lkml/20240520070633.308913-1-slark_xiao@163.com/

There are nothing to fix yet. Great. Then you can resend the Foxconn 
SDX72 introduction work as a series that also includes these mux id 
changes. Just rename this specific patch to something less terrifying. 
Mean, remove the "Fix" word from the subject, please.

Looks like "net: wwan: mhi: make default data link id configurable" 
subject also summarize the reason of the change.

>>> ---
>>>    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..4ca5c845394b 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 ? mhi_dev->mhi_cntrl->link_id : 0);
>>
>> Is it possible to drop the ternary operator and pass the link_id directly?
>>
>>>    }
>>>    
>>>    static void mhi_mbim_remove(struct mhi_device *mhi_dev)

--
Sergey

Re:Re: [PATCH v1 2/2] net: wwan: Fix SDX72 ping failure issue

[Slark Xiao]

At 2024-06-12 06:46:33, "Sergey Ryazanov" <ryazanov.s.a@gmail.com> wrote:
>On 11.06.2024 04:36, Slark Xiao wrote:
>> +More maintainer to this second patch list.
>> 
>> At 2024-06-08 06:28:48, "Sergey Ryazanov" <ryazanov.s.a@gmail.com> wrote:
>>> Hello Slark,
>>>
>>> without the first patch it is close to impossible to understand this
>>> one. Next time please send such tightly connected patches to both
>>> mailing lists.
>>>
>> Sorry for this mistake since it's my first commit about committing code to 2
>> difference area: mhi and mbim. Both the maintainers are difference.
>> In case a new version commit would be created, I would like to ask if
>> should I add both side maintainers on these 2 patches ?
>
>No worries. We finally got both sides of the puzzle. BTW, looks like the 
>first patch still lacks Linux netdev mailing list in the CC.
>
>Usually maintainers are responsible for applying patches to their 
>dedicated repositories (trees), and then eventually for sending them in 
>batch to the main tree. So, if a work consists of two patches, it is 
>better to apply them together to one of the trees. Otherwise, it can 
>cause a build failure in one tree due to lack of required changes that 
>have been applied to other. Sometimes contributors even specify a 
>preferred tree in a cover letter. However, it is still up to maintainers 
>to make a decision which tree is better when a work changes several 
>subsystems.
>

Thanks for your detailed explanation. 
Since this change was modified mainly on mhi side, I prefer to commit it to
 mhi side. 
@loic @mani, what's your opinion?

>>> On 07.06.2024 13:03, 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.
>>>>
>>>> Signed-off-by: Slark Xiao <slark_xiao@163.com>
>>>
>>> Since it a but fix, it needs a 'Fixes:' tag.
>>>
>> Actually, I thought it's a fix for common SDX72 product. But now I think
>> it should be only meet for my SDX72 MBIM product. Previous commit
>> has not been applied. So there is no commit id for "Fixes".
>> But I think I shall include that patch in V2 version.
>> Please ref:
>> https://lore.kernel.org/lkml/20240520070633.308913-1-slark_xiao@163.com/
>
>There are nothing to fix yet. Great. Then you can resend the Foxconn 
>SDX72 introduction work as a series that also includes these mux id 
>changes. Just rename this specific patch to something less terrifying. 
>Mean, remove the "Fix" word from the subject, please.
>
>Looks like "net: wwan: mhi: make default data link id configurable" 
>subject also summarize the reason of the change.
>

Currently I don't know if my previous commit which has been reviewed still
be effective. Since this link_id changes only works for MBIM mode of SDX72.
If keeps the commit of [1], then I will update this patch with v2 version which just update
the subject . If not, then this SDX72 series would have 3 patches: [1] + first patch
+ second patch[v2](or 2 patches: combine [1] with first patch + second patch[v2]).
Please let me know which solution would be better.

Thanks.
>>>> ---
>>>>    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..4ca5c845394b 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 ? mhi_dev->mhi_cntrl->link_id : 0);
>>>
>>> Is it possible to drop the ternary operator and pass the link_id directly?
>>>
>>>>    }
>>>>    
>>>>    static void mhi_mbim_remove(struct mhi_device *mhi_dev)
>
>--
>Sergey
[1] - https://lore.kernel.org/lkml/20240520070633.308913-1-slark_xiao@163.com/

Re: Re: [PATCH v1 2/2] net: wwan: Fix SDX72 ping failure issue

[manivannan.sadhasivam]

On Wed, Jun 12, 2024 at 11:05:38AM +0800, Slark Xiao wrote:
> 
> At 2024-06-12 06:46:33, "Sergey Ryazanov" <ryazanov.s.a@gmail.com> wrote:
> >On 11.06.2024 04:36, Slark Xiao wrote:
> >> +More maintainer to this second patch list.
> >> 
> >> At 2024-06-08 06:28:48, "Sergey Ryazanov" <ryazanov.s.a@gmail.com> wrote:
> >>> Hello Slark,
> >>>
> >>> without the first patch it is close to impossible to understand this
> >>> one. Next time please send such tightly connected patches to both
> >>> mailing lists.
> >>>
> >> Sorry for this mistake since it's my first commit about committing code to 2
> >> difference area: mhi and mbim. Both the maintainers are difference.
> >> In case a new version commit would be created, I would like to ask if
> >> should I add both side maintainers on these 2 patches ?
> >
> >No worries. We finally got both sides of the puzzle. BTW, looks like the 
> >first patch still lacks Linux netdev mailing list in the CC.
> >
> >Usually maintainers are responsible for applying patches to their 
> >dedicated repositories (trees), and then eventually for sending them in 
> >batch to the main tree. So, if a work consists of two patches, it is 
> >better to apply them together to one of the trees. Otherwise, it can 
> >cause a build failure in one tree due to lack of required changes that 
> >have been applied to other. Sometimes contributors even specify a 
> >preferred tree in a cover letter. However, it is still up to maintainers 
> >to make a decision which tree is better when a work changes several 
> >subsystems.
> >
> 
> Thanks for your detailed explanation. 
> Since this change was modified mainly on mhi side, I prefer to commit it to
>  mhi side. 
> @loic @mani, what's your opinion?
> 

There is a build dependency with the MHI patch. So I'll just take both patches
through MHI tree once I get an ACK from WWAN maintainers.

> >>> On 07.06.2024 13:03, 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.
> >>>>
> >>>> Signed-off-by: Slark Xiao <slark_xiao@163.com>
> >>>
> >>> Since it a but fix, it needs a 'Fixes:' tag.
> >>>
> >> Actually, I thought it's a fix for common SDX72 product. But now I think
> >> it should be only meet for my SDX72 MBIM product. Previous commit
> >> has not been applied. So there is no commit id for "Fixes".
> >> But I think I shall include that patch in V2 version.
> >> Please ref:
> >> https://lore.kernel.org/lkml/20240520070633.308913-1-slark_xiao@163.com/
> >
> >There are nothing to fix yet. Great. Then you can resend the Foxconn 
> >SDX72 introduction work as a series that also includes these mux id 
> >changes. Just rename this specific patch to something less terrifying. 
> >Mean, remove the "Fix" word from the subject, please.
> >
> >Looks like "net: wwan: mhi: make default data link id configurable" 
> >subject also summarize the reason of the change.
> >
> 
> Currently I don't know if my previous commit which has been reviewed still
> be effective. Since this link_id changes only works for MBIM mode of SDX72.
> If keeps the commit of [1], then I will update this patch with v2 version which just update
> the subject . If not, then this SDX72 series would have 3 patches: [1] + first patch
> + second patch[v2](or 2 patches: combine [1] with first patch + second patch[v2]).
> Please let me know which solution would be better.
> 

Just send v2 of both patches. There are some comments in the MHI patch as well.

> Thanks.
> >>>> ---
> >>>>    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..4ca5c845394b 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 ? mhi_dev->mhi_cntrl->link_id : 0);
> >>>
> >>> Is it possible to drop the ternary operator and pass the link_id directly?
> >>>

Yeah, just use link_id directly as it will be 0 by default.

- Mani

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