On Wed, Apr 27, 2022 at 4:14 AM Veleta, Moises <moises.veleta@xxxxxxxxx> wrote:
> From: Sergey Ryazanov <ryazanov.s.a@xxxxxxxxx>
> Sent: Tuesday, April 26, 2022 4:06 PM
> To: Veleta, Moises <moises.veleta@xxxxxxxxx>
> Cc: Ricardo Martinez <ricardo.martinez@xxxxxxxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx <netdev@xxxxxxxxxxxxxxx>; linux-wireless@xxxxxxxxxxxxxxx <linux-wireless@xxxxxxxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>; David Miller <davem@xxxxxxxxxxxxx>; Johannes Berg <johannes@xxxxxxxxxxxxxxxx>; Loic Poulain <loic.poulain@xxxxxxxxxx>; Kumar, M Chetan <m.chetan.kumar@xxxxxxxxx>; Devegowda, Chandrashekar <chandrashekar.devegowda@xxxxxxxxx>; linuxwwan <linuxwwan@xxxxxxxxx>; chiranjeevi.rapolu@xxxxxxxxxxxxxxx <chiranjeevi.rapolu@xxxxxxxxxxxxxxx>; Liu, Haijun <haijun.liu@xxxxxxxxxxxx>; Hanania, Amir <amir.hanania@xxxxxxxxx>; Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>; Sharma, Dinesh <dinesh.sharma@xxxxxxxxx>; Lee, Eliot <eliot.lee@xxxxxxxxx>; Jarvinen, Ilpo Johannes <ilpo.johannes.jarvinen@xxxxxxxxx>; Bossart, Pierre-louis <pierre-louis.bossart@xxxxxxxxx>; Sethuraman, Muralidharan <muralidharan.sethuraman@xxxxxxxxx>; Mishra, Soumya Prakash <soumya.prakash.mishra@xxxxxxxxx>; Kancharla, Sreehari <sreehari.kancharla@xxxxxxxxx>; Sahu, Madhusmita <madhusmita.sahu@xxxxxxxxx>
> Subject: Re: [PATCH net-next v6 04/13] net: wwan: t7xx: Add port proxy infrastructure
>
> Hello Moises,
>
> On Tue, Apr 26, 2022 at 10:46 PM Veleta, Moises <moises.veleta@xxxxxxxxx> wrote:
>> From: Sergey Ryazanov <ryazanov.s.a@xxxxxxxxx>
>> Sent: Monday, April 25, 2022 4:53 PM
>> To: Ricardo Martinez <ricardo.martinez@xxxxxxxxxxxxxxx>
>> Cc: netdev@xxxxxxxxxxxxxxx <netdev@xxxxxxxxxxxxxxx>; linux-wireless@xxxxxxxxxxxxxxx <linux-wireless@xxxxxxxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>; David Miller <davem@xxxxxxxxxxxxx>; Johannes Berg <johannes@xxxxxxxxxxxxxxxx>; Loic Poulain <loic.poulain@xxxxxxxxxx>; Kumar, M Chetan <m.chetan.kumar@xxxxxxxxx>; Devegowda, Chandrashekar <chandrashekar.devegowda@xxxxxxxxx>; linuxwwan <linuxwwan@xxxxxxxxx>; chiranjeevi.rapolu@xxxxxxxxxxxxxxx <chiranjeevi.rapolu@xxxxxxxxxxxxxxx>; Liu, Haijun <haijun.liu@xxxxxxxxxxxx>; Hanania, Amir <amir.hanania@xxxxxxxxx>; Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>; Sharma, Dinesh <dinesh.sharma@xxxxxxxxx>; Lee, Eliot <eliot.lee@xxxxxxxxx>; Jarvinen, Ilpo Johannes <ilpo.johannes.jarvinen@xxxxxxxxx>; Veleta, Moises <moises.veleta@xxxxxxxxx>; Bossart, Pierre-louis <pierre-louis.bossart@xxxxxxxxx>; Sethuraman, Muralidharan <muralidharan.sethuraman@xxxxxxxxx>; Mishra, Soumya Prakash <soumya.prakash.mishra@xxxxxxxxx>; Kancharla, Sreehari <sreehari.kancharla@xxxxxxxxx>; Sahu, Madhusmita <madhusmita.sahu@xxxxxxxxx>
>> Subject: Re: [PATCH net-next v6 04/13] net: wwan: t7xx: Add port proxy infrastructure
>>
>> On Fri, Apr 8, 2022 at 1:37 AM Ricardo Martinez
>> <ricardo.martinez@xxxxxxxxxxxxxxx> wrote:
>>> Port-proxy provides a common interface to interact with different types
>>> of ports. Ports export their configuration via `struct t7xx_port` and
>>> operate as defined by `struct port_ops`.
>>>
>>> Signed-off-by: Haijun Liu <haijun.liu@xxxxxxxxxxxx>
>>> Co-developed-by: Chandrashekar Devegowda <chandrashekar.devegowda@xxxxxxxxx>
>>> Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@xxxxxxxxx>
>>> Co-developed-by: Ricardo Martinez <ricardo.martinez@xxxxxxxxxxxxxxx>
>>> Signed-off-by: Ricardo Martinez <ricardo.martinez@xxxxxxxxxxxxxxx>
>>>
>>> From a WWAN framework perspective:
>>> Reviewed-by: Loic Poulain <loic.poulain@xxxxxxxxxx>
>>
>> [skipped]
>>
>>> +int t7xx_port_enqueue_skb(struct t7xx_port *port, struct sk_buff *skb)
>>> +{
>>> + unsigned long flags;
>>> +
>>> + spin_lock_irqsave(&port->rx_wq.lock, flags);
>>> + if (port->rx_skb_list.qlen >= port->rx_length_th) {
>>> + spin_unlock_irqrestore(&port->rx_wq.lock, flags);
>>
>> Probably skb should be freed here before returning. The caller assumes
>> that skb will be consumed even in case of error.
>>
>> [MV] We do not drop port ctrl messages. We keep them and try again later.
>> Whereas WWAN skbs are dropped if conditions are met.
>
> I missed that the WWAN port returns no error when it drops the skb.
> And then I concluded that any failure to process the CCCI message
> should be accomplished with the skb freeing. Now the handling of CCCI
> messages is more clear to me. Thank you for the clarifications!
>
> To avoid similar misinterpretation in the future, I thought that the
> skb freeing in the WWAN port worth a comment. Something to describe
> that despite dropping the message, the return code is zero, indicating
> skb consumption. Similarly in this (t7xx_port_enqueue_skb) function.
> Something like: "return an error here, the caller will try again
> later". And maybe in t7xx_cldma_gpd_rx_from_q() near the loop break
> after the .skb_recv() failure test. Something like: "break message
> processing for now will try this message later".
>
> What do you think?
>
> Yes, that would remove the unintended obfuscation . Unless, you think this approach is inappropriate
> and should be refactored. Otherwise, I will proceed with adding those clarifying comments.
> Thank you.
I am Ok with the current approach. It does not contain obvious errors.
It might look puzzled, but proper comments should solve this.
--
Sergey
