linux-kernel - 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 01:46:33 +0300
From: Sergey Ryazanov <ryazanov.s.a@...il.com>
To: Slark Xiao <slark_xiao@....com>, quic_jhugo@...cinc.com,
 Qiang Yu <quic_qianyu@...cinc.com>
Cc: loic.poulain@...aro.org, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, manivannan.sadhasivam@...aro.org,
 "mhi@...ts.linux.dev" <mhi@...ts.linux.dev>,
 "linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH v1 2/2] net: wwan: Fix SDX72 ping failure issue

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.

>> 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.

>>> ---
>>>    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