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

[manivannan.sadhasivam@xxxxxxxxxx]

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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxx>
> >>>
> >>> 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@xxxxxxx/
> >
> >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

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