Re: [PATCH net-next v4 2/6] net: dsa: Add convenience functions for frame handling

[Mattias Forsblad <mattias.forsblad@gmail.com>]

On 2022-09-06 14:43, Andrew Lunn wrote:
> On Tue, Sep 06, 2022 at 08:34:46AM +0200, Mattias Forsblad wrote:
>> Add common control functions for drivers that need
>> to send and wait for control frames.
> 
> It would be nice to explain why a custom complete is needed. Ideally,
> it should not be needed at all.
> 

My first approach was with without a custom complete as I only used
one single complete instance. However, when migrating the qca8k driver
I noticed they use two different complete instances, one in
qca8k_mgmt_eth_data and one in qca8k_mib_eth_data. This leads
to the suggestion that the qca8k implementation could have several
requests in-flight, thus the custom completion parameter.

>> Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
>> ---
>>  include/net/dsa.h | 13 +++++++++++++
>>  net/dsa/dsa.c     | 28 ++++++++++++++++++++++++++++
>>  net/dsa/dsa2.c    |  2 ++
>>  3 files changed, 43 insertions(+)
>>
>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>> index f2ce12860546..70a358641235 100644
>> --- a/include/net/dsa.h
>> +++ b/include/net/dsa.h
>> @@ -495,6 +495,8 @@ struct dsa_switch {
>>  	unsigned int		max_num_bridges;
>>  
>>  	unsigned int		num_ports;
>> +
>> +	struct completion	inband_done;
>>  };
>>  
>>  static inline struct dsa_port *dsa_to_port(struct dsa_switch *ds, int p)
>> @@ -1390,6 +1392,17 @@ void dsa_tag_drivers_register(struct dsa_tag_driver *dsa_tag_driver_array[],
>>  void dsa_tag_drivers_unregister(struct dsa_tag_driver *dsa_tag_driver_array[],
>>  				unsigned int count);
>>  
>> +int dsa_switch_inband_tx(struct dsa_switch *ds, struct sk_buff *skb,
>> +			 struct completion *completion, unsigned long timeout);
> 
> Blank line please.
> 

Will fix.

>> +static inline void dsa_switch_inband_complete(struct dsa_switch *ds, struct completion *completion)
>> +{
>> +	/* Custom completion? */
>> +	if (completion)
>> +		complete(completion);
>> +	else
>> +		complete(&ds->inband_done);
>> +}
>> +
>>  #define dsa_tag_driver_module_drivers(__dsa_tag_drivers_array, __count)	\
>>  static int __init dsa_tag_driver_module_init(void)			\
>>  {									\
>> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
>> index be7b320cda76..2d7add779b6f 100644
>> --- a/net/dsa/dsa.c
>> +++ b/net/dsa/dsa.c
>> @@ -324,6 +324,34 @@ int dsa_switch_resume(struct dsa_switch *ds)
>>  EXPORT_SYMBOL_GPL(dsa_switch_resume);
>>  #endif
>>  
>> +int dsa_switch_inband_tx(struct dsa_switch *ds, struct sk_buff *skb,
>> +			 struct completion *completion, unsigned long timeout)
>> +{
>> +	int ret;
>> +	struct completion *com;
> 
> Reverse christmas tree. Longest lines first.
> 

Duh, again? Sorry, will fix.

>> +
>> +	/* Custom completion? */
>> +	if (completion)
>> +		com = completion;
>> +	else
>> +		com = &ds->inband_done;
>> +
>> +	reinit_completion(com);
>> +
>> +	if (skb)
>> +		dev_queue_xmit(skb);
>> +
>> +	ret = wait_for_completion_timeout(com, msecs_to_jiffies(timeout));
>> +	if (ret <= 0) {
>> +		dev_dbg(ds->dev, "DSA inband: timeout waiting for answer\n");
>> +
>> +		return -ETIMEDOUT;
>> +	}
> 
> It looks like wait_for_completion_timeout() can return a negative
> error code. You should return that error code, not replace it with
> -ETIMEDOUT. If it returns 0, then it has timed out, and returning
> -ETIMEDOUT does make sense. If the completion is indicated before the
> timeout, the return value is the remaining time. So you can return a
> positive number here. It is worth documenting that, since a common
> patterns is:
> 
> 	err = dsa_switch_inband_tx()
> 	if (err)
> 		return err;
> 
> does not work in this case.
> 
>      Andrew

Will fix.
	
	Mattias