Patchwork stmmac: fix driver features

login
register
mail settings
Submitter Giuseppe CAVALLARO
Date Feb. 9, 2012, 10:56 a.m.
Message ID <1328784993-31733-1-git-send-email-peppe.cavallaro@st.com>
Download mbox | patch
Permalink /patch/140366/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Giuseppe CAVALLARO - Feb. 9, 2012, 10:56 a.m.
New GMAC chips can set the tx_coe and rx_csum
flags by looking at the HW cap register and this
happens during the open.
This patch fixes the stmmac_fix_feature function that
in some cases assumes that there is no HW csum
because no flags are passed through the platform.
As soon as the open method is called then the
stmmac_fix_feature could want to turn-on the NETIF_F_RXCSUM
or NETIF_F_ALL_CSUM.

Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
David Miller - Feb. 9, 2012, 8:35 p.m.
From: Giuseppe CAVALLARO <peppe.cavallaro@st.com>
Date: Thu,  9 Feb 2012 11:56:33 +0100

> New GMAC chips can set the tx_coe and rx_csum
> flags by looking at the HW cap register and this
> happens during the open.
> This patch fixes the stmmac_fix_feature function that
> in some cases assumes that there is no HW csum
> because no flags are passed through the platform.
> As soon as the open method is called then the
> stmmac_fix_feature could want to turn-on the NETIF_F_RXCSUM
> or NETIF_F_ALL_CSUM.
> 
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>

This is not the purpose of the fix_features method, it's meant to
ensure that the settings are valid.

It's not meant to "catch up" with settings you store in the internal
datastructures of your driver.

You need to do this at probe time, where the initial ->hw_features
and ->features values are set.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Giuseppe CAVALLARO - Feb. 10, 2012, 7:08 a.m.
Hello David

On 2/9/2012 9:35 PM, David Miller wrote:
> This is not the purpose of the fix_features method, it's meant to
> ensure that the settings are valid.
> 
> It's not meant to "catch up" with settings you store in the internal
> datastructures of your driver.
> 
> You need to do this at probe time, where the initial ->hw_features
> and ->features values are set.

Initially the driver HW features are indeed set in the probe but in the
stmmac_open function, after looking at the HW cap reg, some parameters,
for example the HW csum, can be overridden and the
netdev_update_features is called. IIUC the netdev_update_features calls
the driver's ndo_fix_features. For this reason I improved the
stmmac_fix_feature function to cover more setting. Anyway, if I cannot
use this function I should move from the open to the probe the logic to
manage the MAC identification and HW cap register. What do you suggest?

Many thanks for you review
Regards
Peppe
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Feb. 10, 2012, 7:40 a.m.
From: Giuseppe CAVALLARO <peppe.cavallaro@st.com>
Date: Fri, 10 Feb 2012 08:08:54 +0100

> Hello David
> 
> On 2/9/2012 9:35 PM, David Miller wrote:
>> This is not the purpose of the fix_features method, it's meant to
>> ensure that the settings are valid.
>> 
>> It's not meant to "catch up" with settings you store in the internal
>> datastructures of your driver.
>> 
>> You need to do this at probe time, where the initial ->hw_features
>> and ->features values are set.
> 
> Initially the driver HW features are indeed set in the probe but in the
> stmmac_open function, after looking at the HW cap reg, some parameters,
> for example the HW csum, can be overridden and the
> netdev_update_features is called. IIUC the netdev_update_features calls
> the driver's ndo_fix_features. For this reason I improved the
> stmmac_fix_feature function to cover more setting. Anyway, if I cannot
> use this function I should move from the open to the probe the logic to
> manage the MAC identification and HW cap register. What do you suggest?

You should not be determining chip features in your open method,
such work belongs in your device probe.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Giuseppe CAVALLARO - Feb. 10, 2012, 8:40 a.m.
On 2/10/2012 8:40 AM, David Miller wrote:
> From: Giuseppe CAVALLARO <peppe.cavallaro@st.com>
> Date: Fri, 10 Feb 2012 08:08:54 +0100
> 
>> Hello David
>>
>> On 2/9/2012 9:35 PM, David Miller wrote:
>>> This is not the purpose of the fix_features method, it's meant to
>>> ensure that the settings are valid.
>>>
>>> It's not meant to "catch up" with settings you store in the internal
>>> datastructures of your driver.
>>>
>>> You need to do this at probe time, where the initial ->hw_features
>>> and ->features values are set.
>>
>> Initially the driver HW features are indeed set in the probe but in the
>> stmmac_open function, after looking at the HW cap reg, some parameters,
>> for example the HW csum, can be overridden and the
>> netdev_update_features is called. IIUC the netdev_update_features calls
>> the driver's ndo_fix_features. For this reason I improved the
>> stmmac_fix_feature function to cover more setting. Anyway, if I cannot
>> use this function I should move from the open to the probe the logic to
>> manage the MAC identification and HW cap register. What do you suggest?
> 
> You should not be determining chip features in your open method,
> such work belongs in your device probe.
> 
ok, I'll rework this and send you all the patches again in a bundle.

peppe
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 36ee77f..e03a873 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1541,8 +1541,13 @@  static netdev_features_t stmmac_fix_features(struct net_device *dev,
 
 	if (!priv->rx_coe)
 		features &= ~NETIF_F_RXCSUM;
+	else
+		features |= NETIF_F_RXCSUM;
+
 	if (!priv->plat->tx_coe)
 		features &= ~NETIF_F_ALL_CSUM;
+	else
+		features |= NETIF_F_ALL_CSUM;
 
 	/* Some GMAC devices have a bugged Jumbo frame support that
 	 * needs to have the Tx COE disabled for oversized frames