netdev - Re: [PATCH net-next v13 6/6] net: dsa: qca8k: Use new convenience functions

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] [day] [month] [year] [list]

Message-ID: <12edaefc-89a2-f231-156e-5dbe198ae6f6@gmail.com>
Date:   Mon, 19 Sep 2022 07:18:24 +0200
From:   Mattias Forsblad <mattias.forsblad@...il.com>
To:     Christian Marangi <ansuelsmth@...il.com>
Cc:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, linux@...linux.org.uk
Subject: Re: [PATCH net-next v13 6/6] net: dsa: qca8k: Use new convenience
 functions

On 2022-09-16 08:09, Christian Marangi wrote:
>> @@ -606,17 +587,12 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
>>  		kfree_skb(read_skb);
>>  	}
>>  exit:
>> -	reinit_completion(&mgmt_eth_data->rw_done);
>> -
>>  	/* Increment seq_num and set it in the clear pkt */
>>  	mgmt_eth_data->seq++;
>>  	qca8k_mdio_header_fill_seq_num(clear_skb, mgmt_eth_data->seq);
>>  	mgmt_eth_data->ack = false;
>>  
>> -	dev_queue_xmit(clear_skb);
>> -
>> -	wait_for_completion_timeout(&mgmt_eth_data->rw_done,
>> -				    QCA8K_ETHERNET_TIMEOUT);
>> +	ret = dsa_switch_inband_tx(ds, clear_skb, &mgmt_eth_data->rw_done, QCA8K_ETHERNET_TIMEOUT);
> 
> This cause the breakage of qca8k!
> 
> The clear_skb is used to clean a state but is optional and we should not
> check exit value.
> 
> On top of that this overwrites the mdio return value from the read
> condition.
> 
> ret = mgmt_eth_data->data[0] & QCA8K_MDIO_MASTER_DATA_MASK;
> 
> This should be changed to just
> 
> dsa_switch_inband_tx(ds, clear_skb, &mgmt_eth_data->rw_done, QCA8K_ETHERNET_TIMEOUT);
> 
> Also considering the majority of the driver is alligned to 80 column can
> you wrap these new function to that? (personal taste)
>

Thanks for the testing, I'll fix the issue you've found and do a respin.

/Mattias

 
>>  
>>  	mutex_unlock(&mgmt_eth_data->mutex);
>>  
>> @@ -1528,7 +1504,7 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
>>  exit:
>>  	/* Complete on receiving all the mib packet */
>>  	if (refcount_dec_and_test(&mib_eth_data->port_parsed))
>> -		complete(&mib_eth_data->rw_done);
>> +		dsa_switch_inband_complete(ds, &mib_eth_data->rw_done);
>>  }
>>  
>>  static int
>> @@ -1543,8 +1519,6 @@ qca8k_get_ethtool_stats_eth(struct dsa_switch *ds, int port, u64 *data)
>>  
>>  	mutex_lock(&mib_eth_data->mutex);
>>  
>> -	reinit_completion(&mib_eth_data->rw_done);
>> -
>>  	mib_eth_data->req_port = dp->index;
>>  	mib_eth_data->data = data;
>>  	refcount_set(&mib_eth_data->port_parsed, QCA8K_NUM_PORTS);
>> @@ -1562,8 +1536,7 @@ qca8k_get_ethtool_stats_eth(struct dsa_switch *ds, int port, u64 *data)
>>  	if (ret)
>>  		goto exit;
>>  
>> -	ret = wait_for_completion_timeout(&mib_eth_data->rw_done, QCA8K_ETHERNET_TIMEOUT);
>> -
>> +	ret = dsa_switch_inband_tx(ds, NULL, &mib_eth_data->rw_done, QCA8K_ETHERNET_TIMEOUT);
>>  exit:
>>  	mutex_unlock(&mib_eth_data->mutex);
>>  
>> -- 
>> 2.25.1
>>
>