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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 12 Jun 2024 11:05:38 +0800 (CST)
From: "Slark Xiao" <slark_xiao@....com>
To: "Sergey Ryazanov" <ryazanov.s.a@...il.com>, 
	"Manivannan Sadhasivam" <mani@...nel.org>, 
	"manivannan.sadhasivam@...aro.org" <manivannan.sadhasivam@...aro.org>, 
	"Loic Poulain" <loic.poulain@...aro.org>
Cc: quic_jhugo@...cinc.com, "Qiang Yu" <quic_qianyu@...cinc.com>, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	"mhi@...ts.linux.dev" <mhi@...ts.linux.dev>, 
	"linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>
Subject: Re:Re: [PATCH v1 2/2] net: wwan: Fix SDX72 ping failure issue


At 2024-06-12 06:46:33, "Sergey Ryazanov" <ryazanov.s.a@...il.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@...il.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@....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/