Re: [PATCH net-next v2] net: wwan: Add WWAN sahara port type

Re: [PATCH net-next v2] net: wwan: Add WWAN sahara port type

[Sergey Ryazanov]

+Manivannan

Hello Jerry,

this version looks a way better, still there is one minor thing to 
improve. See below.

Manivannan, Loic, could you advice is it Ok to export that SAHARA port 
as is?

On 20.11.2024 11:39, Jerry Meng wrote:
> Add a Sahara protocol-based interface for downloading ramdump
> from Qualcomm modems in SBL ramdump mode.
> 
> Signed-off-by: Jerry Meng <jerry.meng.lk@quectel.com>
> ---
> v1 -> v2:
> 	- Fix errors checked by checkpatch.pl, mainly change indentation from space to tab
> 	- change my email acount to fit git-send-email
> 
>   drivers/net/wwan/mhi_wwan_ctrl.c | 1 +
>   drivers/net/wwan/wwan_core.c     | 4 ++++
>   include/linux/wwan.h             | 2 ++
>   3 files changed, 7 insertions(+)
> 
> diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
> index e9f979d2d..082090ae5 100644
> --- a/drivers/net/wwan/mhi_wwan_ctrl.c
> +++ b/drivers/net/wwan/mhi_wwan_ctrl.c
> @@ -263,6 +263,7 @@ static const struct mhi_device_id mhi_wwan_ctrl_match_table[] = {
>   	{ .chan = "QMI", .driver_data = WWAN_PORT_QMI },
>   	{ .chan = "DIAG", .driver_data = WWAN_PORT_QCDM },
>   	{ .chan = "FIREHOSE", .driver_data = WWAN_PORT_FIREHOSE },
> +	{ .chan = "SAHARA", .driver_data = WWAN_PORT_SAHARA},
                                                            ^
The space is still missing between WWAN_PORT_SAHARA and trailing '}'. 
Please follow the format of existing table entries.

>   	{},
>   };
>   MODULE_DEVICE_TABLE(mhi, mhi_wwan_ctrl_match_table);
> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> index a51e27559..5eb0d6de3 100644
> --- a/drivers/net/wwan/wwan_core.c
> +++ b/drivers/net/wwan/wwan_core.c
> @@ -342,6 +342,10 @@ static const struct {
>   		.name = "MIPC",
>   		.devsuf = "mipc",
>   	},
> +	[WWAN_PORT_SAHARA] = {
> +		.name = "SAHARA",
> +		.devsuf = "sahara",
> +	},
>   };
>   
>   static ssize_t type_show(struct device *dev, struct device_attribute *attr,
> diff --git a/include/linux/wwan.h b/include/linux/wwan.h
> index 79c781875..b0ea276f2 100644
> --- a/include/linux/wwan.h
> +++ b/include/linux/wwan.h
> @@ -19,6 +19,7 @@
>   * @WWAN_PORT_FASTBOOT: Fastboot protocol control
>   * @WWAN_PORT_ADB: ADB protocol control
>   * @WWAN_PORT_MIPC: MTK MIPC diagnostic interface
> + * @WWAN_PORT_SAHARA: Sahara protocol-based interface for downloading ramdump from Qualcomm modems
>   *
>   * @WWAN_PORT_MAX: Highest supported port types
>   * @WWAN_PORT_UNKNOWN: Special value to indicate an unknown port type
> @@ -34,6 +35,7 @@ enum wwan_port_type {
>   	WWAN_PORT_FASTBOOT,
>   	WWAN_PORT_ADB,
>   	WWAN_PORT_MIPC,
> +	WWAN_PORT_SAHARA,
>   
>   	/* Add new port types above this line */
>   

Re: [PATCH net-next v2] net: wwan: Add WWAN sahara port type

[Jeffrey Hugo]

On 11/20/2024 1:36 PM, Sergey Ryazanov wrote:
> +Manivannan
> 
> Hello Jerry,
> 
> this version looks a way better, still there is one minor thing to 
> improve. See below.
> 
> Manivannan, Loic, could you advice is it Ok to export that SAHARA port 
> as is?

I'm against this.

There is an in-kernel Sahara implementation, which is going to be used 
by QDU100.  If WWAN is going to own the "SAHARA" MHI channel name, then 
no one else can use it which will conflict with QDU100.

I expect the in-kernel implementation can be leveraged for this.

-Jeff

Re: [PATCH net-next v2] net: wwan: Add WWAN sahara port type

[Loic Poulain]

On Wed, 20 Nov 2024 at 22:48, Jeffrey Hugo <quic_jhugo@quicinc.com> wrote:
>
> On 11/20/2024 1:36 PM, Sergey Ryazanov wrote:
> > +Manivannan
> >
> > Hello Jerry,
> >
> > this version looks a way better, still there is one minor thing to
> > improve. See below.
> >
> > Manivannan, Loic, could you advice is it Ok to export that SAHARA port
> > as is?
>
> I'm against this.
>
> There is an in-kernel Sahara implementation, which is going to be used
> by QDU100.  If WWAN is going to own the "SAHARA" MHI channel name, then
> no one else can use it which will conflict with QDU100.
>
> I expect the in-kernel implementation can be leveraged for this.

Fair enough, actually the same change has already been discussed two
years ago, and we agreed that it should not be exposed as a WWAN
control port:
https://lore.kernel.org/netdev/CAMZdPi_7KGx69s5tFumkswVXiQSdxXZjDXT5f9njRnBNz1k-VA@mail.gmail.com/#t

Regards,
Loic

Re: [PATCH net-next v2] net: wwan: Add WWAN sahara port type

[Sergey Ryazanov]

+Jiri

Hi Loic,

On 21.11.2024 11:08, Loic Poulain wrote:
> On Wed, 20 Nov 2024 at 22:48, Jeffrey Hugo <quic_jhugo@quicinc.com> wrote:
>> On 11/20/2024 1:36 PM, Sergey Ryazanov wrote:
>>> +Manivannan
>>>
>>> Hello Jerry,
>>>
>>> this version looks a way better, still there is one minor thing to
>>> improve. See below.
>>>
>>> Manivannan, Loic, could you advice is it Ok to export that SAHARA port
>>> as is?
>>
>> I'm against this.
>>
>> There is an in-kernel Sahara implementation, which is going to be used
>> by QDU100.  If WWAN is going to own the "SAHARA" MHI channel name, then
>> no one else can use it which will conflict with QDU100.
>>
>> I expect the in-kernel implementation can be leveraged for this.
> 
> Fair enough, actually the same change has already been discussed two
> years ago, and we agreed that it should not be exposed as a WWAN
> control port:
> https://lore.kernel.org/netdev/CAMZdPi_7KGx69s5tFumkswVXiQSdxXZjDXT5f9njRnBNz1k-VA@mail.gmail.com/#t

Thank you for reminding us about that conversation. There you have 
shared a good thought, that the WWAN framework suppose to export mostly 
management ports. And all other debug/dump/reflash features should be 
implemented using corresponding kernel APIs.

Last year, more port types were merged. As part of the T7xx driver 
development. Specifically it were Fastboot, ADB, and MIPC. See:
- 495e7c8e9601 wwan: core: Add WWAN ADB and MIPC port type
- e3caf184107a wwan: core: Add WWAN fastboot port type

If ADB somehow could be considered a management interface. MIPC and 
Fastboot are firmware management interfaces. And I recall a discussion 
regarding the Fastboot implementation and there was a NACK from someone 
from devlink subsystem.

Personally, I prefer the firmware management/debugging operations being 
implemented using a generic kernel API like it was done in IOSM. And we 
are suggesting contributors to use the existing kernel API instead of 
exposing RAW interfaces. On another hand, devlink developers are not 
happy to see this kind of devlink usage.

Loic, do you have any idea how to solve this puzzle? And how do you 
think, shall we do something regarding the already introduced 
Fastboot/ADB/MIPC port types?

--
Sergey

Re: [PATCH net-next v2] net: wwan: Add WWAN sahara port type

[Sergey Ryazanov]

Hi Jeffrey,

On 20.11.2024 23:48, Jeffrey Hugo wrote:
> On 11/20/2024 1:36 PM, Sergey Ryazanov wrote:
>> +Manivannan
>>
>> Hello Jerry,
>>
>> this version looks a way better, still there is one minor thing to 
>> improve. See below.
>>
>> Manivannan, Loic, could you advice is it Ok to export that SAHARA port 
>> as is?
> 
> I'm against this.
> 
> There is an in-kernel Sahara implementation, which is going to be used 
> by QDU100.  If WWAN is going to own the "SAHARA" MHI channel name, then 
> no one else can use it which will conflict with QDU100.
> 
> I expect the in-kernel implementation can be leveraged for this.

Make sense. Can you share a link to this in-kernel implementation? I've 
searched through the code and found nothing similar. Is it merged or has 
it a different name?

--
Sergey

Re: [PATCH net-next v2] net: wwan: Add WWAN sahara port type

[Jeffrey Hugo]

On 11/21/2024 3:53 PM, Sergey Ryazanov wrote:
> Hi Jeffrey,
> 
> On 20.11.2024 23:48, Jeffrey Hugo wrote:
>> On 11/20/2024 1:36 PM, Sergey Ryazanov wrote:
>>> +Manivannan
>>>
>>> Hello Jerry,
>>>
>>> this version looks a way better, still there is one minor thing to 
>>> improve. See below.
>>>
>>> Manivannan, Loic, could you advice is it Ok to export that SAHARA 
>>> port as is?
>>
>> I'm against this.
>>
>> There is an in-kernel Sahara implementation, which is going to be used 
>> by QDU100.  If WWAN is going to own the "SAHARA" MHI channel name, 
>> then no one else can use it which will conflict with QDU100.
>>
>> I expect the in-kernel implementation can be leveraged for this.
> 
> Make sense. Can you share a link to this in-kernel implementation? I've 
> searched through the code and found nothing similar. Is it merged or has 
> it a different name?

drivers/accel/qaic/sahara.c

-Jeff

Re: [PATCH net-next v2] net: wwan: Add WWAN sahara port type

[Sergey Ryazanov]

On 22.11.2024 01:02, Jeffrey Hugo wrote:
> On 11/21/2024 3:53 PM, Sergey Ryazanov wrote:
>> On 20.11.2024 23:48, Jeffrey Hugo wrote:
>>> On 11/20/2024 1:36 PM, Sergey Ryazanov wrote:
>>>> +Manivannan
>>>>
>>>> Hello Jerry,
>>>>
>>>> this version looks a way better, still there is one minor thing to 
>>>> improve. See below.
>>>>
>>>> Manivannan, Loic, could you advice is it Ok to export that SAHARA 
>>>> port as is?
>>>
>>> I'm against this.
>>>
>>> There is an in-kernel Sahara implementation, which is going to be 
>>> used by QDU100.  If WWAN is going to own the "SAHARA" MHI channel 
>>> name, then no one else can use it which will conflict with QDU100.
>>>
>>> I expect the in-kernel implementation can be leveraged for this.
>>
>> Make sense. Can you share a link to this in-kernel implementation? 
>> I've searched through the code and found nothing similar. Is it merged 
>> or has it a different name?
> 
> drivers/accel/qaic/sahara.c

I was searching for SAHARA and for sahara in the log, however it was 
introduced as Sahara. Next time will use case insensitive search :) 
Thank you for the clue.

--
Sergey