Re: [PATCH 2/2] net: phy: ensure autoneg is configured when resuming a phydev
On 03.12.2018 11:43, Anssi Hannula wrote:
> On 1.12.2018 0:16, Heiner Kallweit wrote:
>> On 30.11.2018 19:45, Anssi Hannula wrote:
>>> When a PHY_HALTED phydev is resumed by phy_start(), it is set to
>>> PHY_RESUMING to wait for the link to come up.
>>>
>>> However, if the phydev was put to PHY_HALTED (by e.g. phy_stop()) before
>>> autonegotiation was ever started by phy_state_machine(), autonegotiation
>>> remains unconfigured, i.e. phy_config_aneg() is never called.
>>>
>> phy_stop() should only be called once the PHY was started. But it's
>> right that we don't have full control over it because misbehaving
>> drivers may call phy_stop() even if the PHY hasn't been started yet.
>
> Having run phy_start() does not guarantee that the phy_state_machine()
> has been run at least once, though.
>
> I was able to reproduce the issue by calling phy_start(); phy_stop().
> Then on the next phy_start() autoneg is not configured.
>
Right, phy_start() schedules the state machine run, so there is a small
window where this can happen. Did you experience this in any real-life
scenario?
>> I think phy_error() and phy_stop() should be extended to set the state
>> to PHY_HALTED only if the PHY is in a started state, means:
>> don't touch the state if it's DOWN, READY, or HALTED.
>> In case of phy_error() it's more a precaution, because it's used within
>> phylib only and AFAICS it can be triggered from a started state only.
>
> This wouldn't fix the issue that occurs when phy_stop() is called right
> after phy_start(), though, as PHY is in UP state then.
>
After having removed state AN I was thinking already whether we really
need to have separate states READY and HALTED. I think we wouldn't
have this scenario if phy_stop() and phy_error() would set the state
to READY. Merging the two states requires some upfront work, but I have
some ideas in the back of my mind.
Overall this should be cleaner than the proposed workaround.
>>
>> So yes, there is a theoretical issue. But as you wrote already, I think
>> there's a better way to deal with it.
>>
>> For checking whether PHY is in a started state you could use the helper
>> which is being discussed here:
>> https://marc.info/?l=linux-netdev=154360368104946=2
>>
>>
>>> Fix that by going to PHY_UP instead of PHY_RESUMING if autonegotiation
>>> has never been configured.
>>>
>>> Signed-off-by: Anssi Hannula
>>> ---
>>>
>>> This doesn't feel as clean is I'd like to, though. Maybe there is a
>>> better way?
>>>
>>>
>>> drivers/net/phy/phy.c | 11 ++-
>>> include/linux/phy.h | 2 ++
>>> 2 files changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>>> index d46858694db9..7a650cce7599 100644
>>> --- a/drivers/net/phy/phy.c
>>> +++ b/drivers/net/phy/phy.c
>>> @@ -553,6 +553,8 @@ int phy_start_aneg(struct phy_device *phydev)
>>> if (err < 0)
>>> goto out_unlock;
>>>
>>> + phydev->autoneg_configured = 1;
>>> +
>>> if (phydev->state != PHY_HALTED) {
>>> if (AUTONEG_ENABLE == phydev->autoneg) {
>>> err = phy_check_link_status(phydev);
>>> @@ -893,7 +895,14 @@ void phy_start(struct phy_device *phydev)
>>> break;
>>> }
>>>
>>> - phydev->state = PHY_RESUMING;
>>> + /* if autoneg/forcing was never configured, go back to PHY_UP
>>> +* to make that happen
>>> +*/
>>> + if (!phydev->autoneg_configured)
>>> + phydev->state = PHY_UP;
>>> + else
>>> + phydev->state = PHY_RESUMING;
>>> +
>>> break;
>>> default:
>>> break;
>>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>>> index 8f927246acdb..b306d5ed9d09 100644
>>> --- a/include/linux/phy.h
>>> +++ b/include/linux/phy.h
>>> @@ -389,6 +389,8 @@ struct phy_device {
>>> unsigned loopback_enabled:1;
>>>
>>> unsigned autoneg:1;
>>> + /* autoneg has been configured at least once */
>>> + unsigned autoneg_configured:1;
>>> /* The most recently read link state */
>>> unsigned link:1;
>>>
>>>
>
[PATCH net] net: phy: don't allow __set_phy_supported to add unsupported modes
Currently __set_phy_supported allows to add modes w/o checking whether
the PHY supports them. This is wrong, it should never add modes but
only remove modes we don't want to support.
The commit marked as fixed didn't do anything wrong, it just copied
existing functionality to the helper which is being fixed now.
Fixes: f3a6bd393c2c ("phylib: Add phy_set_max_speed helper")
Signed-off-by: Heiner Kallweit
---
This will cause a merge conflict once net is merged into net-next.
And the fix will need minor tweaking when being backported to
older kernel versions.
---
drivers/net/phy/phy_device.c | 19 ---
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 23ee3967c..18e92c19c 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1880,20 +1880,17 @@ EXPORT_SYMBOL(genphy_loopback);
static int __set_phy_supported(struct phy_device *phydev, u32 max_speed)
{
- phydev->supported &= ~(PHY_1000BT_FEATURES | PHY_100BT_FEATURES |
- PHY_10BT_FEATURES);
-
switch (max_speed) {
- default:
- return -ENOTSUPP;
- case SPEED_1000:
- phydev->supported |= PHY_1000BT_FEATURES;
+ case SPEED_10:
+ phydev->supported &= ~PHY_100BT_FEATURES;
/* fall through */
case SPEED_100:
- phydev->supported |= PHY_100BT_FEATURES;
- /* fall through */
- case SPEED_10:
- phydev->supported |= PHY_10BT_FEATURES;
+ phydev->supported &= ~PHY_1000BT_FEATURES;
+ break;
+ case SPEED_1000:
+ break;
+ default:
+ return -ENOTSUPP;
}
return 0;
--
2.19.2
[PATCH net-next] net: phy: don't allow __set_phy_supported to add unsupported modes
Currently __set_phy_supported allows to add modes w/o checking whether
the PHY supports them. This is wrong, it should never add modes but
only remove modes we don't want to support.
Signed-off-by: Heiner Kallweit
---
I'll submit the same functionality for net, but it needs a different
implementation due to linkmode bitmaps having been added meanwhile.
This will cause a merge conflict once net is merged into net-next.
---
drivers/net/phy/phy_device.c | 42
1 file changed, 14 insertions(+), 28 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 0904002b1..40404a8f5 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1898,37 +1898,23 @@ EXPORT_SYMBOL(genphy_loopback);
static int __set_phy_supported(struct phy_device *phydev, u32 max_speed)
{
- __ETHTOOL_DECLARE_LINK_MODE_MASK(speeds) = { 0, };
-
- linkmode_set_bit_array(phy_10_100_features_array,
- ARRAY_SIZE(phy_10_100_features_array),
- speeds);
- linkmode_set_bit_array(phy_gbit_features_array,
- ARRAY_SIZE(phy_gbit_features_array),
- speeds);
-
- linkmode_andnot(phydev->supported, phydev->supported, speeds);
-
switch (max_speed) {
- default:
- return -ENOTSUPP;
- case SPEED_1000:
- linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
-phydev->supported);
- linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
-phydev->supported);
+ case SPEED_10:
+ linkmode_clear_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
+ phydev->supported);
+ linkmode_clear_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
+ phydev->supported);
/* fall through */
case SPEED_100:
- linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
-phydev->supported);
- linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
-phydev->supported);
- /* fall through */
- case SPEED_10:
- linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
-phydev->supported);
- linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT,
-phydev->supported);
+ linkmode_clear_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
+ phydev->supported);
+ linkmode_clear_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+ phydev->supported);
+ break;
+ case SPEED_1000:
+ break;
+ default:
+ return -ENOTSUPP;
}
return 0;
--
2.19.2
Re: [PATCH 2/2] net: phy: ensure autoneg is configured when resuming a phydev
On 30.11.2018 19:45, Anssi Hannula wrote:
> When a PHY_HALTED phydev is resumed by phy_start(), it is set to
> PHY_RESUMING to wait for the link to come up.
>
> However, if the phydev was put to PHY_HALTED (by e.g. phy_stop()) before
> autonegotiation was ever started by phy_state_machine(), autonegotiation
> remains unconfigured, i.e. phy_config_aneg() is never called.
>
phy_stop() should only be called once the PHY was started. But it's
right that we don't have full control over it because misbehaving
drivers may call phy_stop() even if the PHY hasn't been started yet.
I think phy_error() and phy_stop() should be extended to set the state
to PHY_HALTED only if the PHY is in a started state, means:
don't touch the state if it's DOWN, READY, or HALTED.
In case of phy_error() it's more a precaution, because it's used within
phylib only and AFAICS it can be triggered from a started state only.
So yes, there is a theoretical issue. But as you wrote already, I think
there's a better way to deal with it.
For checking whether PHY is in a started state you could use the helper
which is being discussed here:
https://marc.info/?l=linux-netdev=154360368104946=2
> Fix that by going to PHY_UP instead of PHY_RESUMING if autonegotiation
> has never been configured.
>
> Signed-off-by: Anssi Hannula
> ---
>
> This doesn't feel as clean is I'd like to, though. Maybe there is a
> better way?
>
>
> drivers/net/phy/phy.c | 11 ++-
> include/linux/phy.h | 2 ++
> 2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index d46858694db9..7a650cce7599 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -553,6 +553,8 @@ int phy_start_aneg(struct phy_device *phydev)
> if (err < 0)
> goto out_unlock;
>
> + phydev->autoneg_configured = 1;
> +
> if (phydev->state != PHY_HALTED) {
> if (AUTONEG_ENABLE == phydev->autoneg) {
> err = phy_check_link_status(phydev);
> @@ -893,7 +895,14 @@ void phy_start(struct phy_device *phydev)
> break;
> }
>
> - phydev->state = PHY_RESUMING;
> + /* if autoneg/forcing was never configured, go back to PHY_UP
> + * to make that happen
> + */
> + if (!phydev->autoneg_configured)
> + phydev->state = PHY_UP;
> + else
> + phydev->state = PHY_RESUMING;
> +
> break;
> default:
> break;
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 8f927246acdb..b306d5ed9d09 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -389,6 +389,8 @@ struct phy_device {
> unsigned loopback_enabled:1;
>
> unsigned autoneg:1;
> + /* autoneg has been configured at least once */
> + unsigned autoneg_configured:1;
> /* The most recently read link state */
> unsigned link:1;
>
>
[PATCH net-next] net: phy: improve generic EEE ethtool functions
So far the two functions consider neither member eee_enabled nor
eee_active. Therefore network drivers have to do this in some kind
of glue code. I think this can be avoided.
Getting EEE parameters:
When not advertising any EEE mode, we can't consider EEE to be enabled.
Therefore interpret "EEE enabled" as "we advertise at least one EEE
mode". It's similar with "EEE active": interpret it as "EEE modes
advertised by both link partner have at least one mode in common".
Setting EEE parameters:
If eee_enabled isn't set, don't advertise any EEE mode and restart
aneg if needed to switch off EEE. If eee_enabled is set and
data->advertised is empty (e.g. because EEE was disabled), advertise
everything we support as default. This way EEE can easily switched
on/off by doing ethtool --set-eee eee on/off, w/o any additional
parameters.
The changes to both functions shouldn't break any existing user.
Once the changes have been applied, at least some users can be
simplified.
Signed-off-by: Heiner Kallweit
---
drivers/net/phy/phy.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 376a0d8a2..e1a1e54ba 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1144,6 +1144,7 @@ int phy_ethtool_get_eee(struct phy_device *phydev, struct
ethtool_eee *data)
if (val < 0)
return val;
data->advertised = mmd_eee_adv_to_ethtool_adv_t(val);
+ data->eee_enabled = !!data->advertised;
/* Get LP advertisement EEE */
val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_LPABLE);
@@ -1151,6 +1152,8 @@ int phy_ethtool_get_eee(struct phy_device *phydev, struct
ethtool_eee *data)
return val;
data->lp_advertised = mmd_eee_adv_to_ethtool_adv_t(val);
+ data->eee_active = !!(data->advertised & data->lp_advertised);
+
return 0;
}
EXPORT_SYMBOL(phy_ethtool_get_eee);
@@ -1164,7 +1167,7 @@ EXPORT_SYMBOL(phy_ethtool_get_eee);
*/
int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_eee *data)
{
- int cap, old_adv, adv, ret;
+ int cap, old_adv, adv = 0, ret;
if (!phydev->drv)
return -EIO;
@@ -1178,10 +1181,12 @@ int phy_ethtool_set_eee(struct phy_device *phydev,
struct ethtool_eee *data)
if (old_adv < 0)
return old_adv;
- adv = ethtool_adv_to_mmd_eee_adv_t(data->advertised) & cap;
-
- /* Mask prohibited EEE modes */
- adv &= ~phydev->eee_broken_modes;
+ if (data->eee_enabled) {
+ adv = !data->advertised ? cap :
+ ethtool_adv_to_mmd_eee_adv_t(data->advertised) & cap;
+ /* Mask prohibited EEE modes */
+ adv &= ~phydev->eee_broken_modes;
+ }
if (old_adv != adv) {
ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV, adv);
--
2.19.2
[PATCH net-next] r8169: remove unneeded mmiowb barriers
writex() has implicit barriers, that's what makes it different from
writex_relaxed(). Therefore these calls to mmiowb() can be removed.
Signed-off-by: Heiner Kallweit
---
drivers/net/ethernet/realtek/r8169.c | 8 +---
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index 4114c2712..bb1847fd6 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1283,13 +1283,11 @@ static u16 rtl_get_events(struct rtl8169_private *tp)
static void rtl_ack_events(struct rtl8169_private *tp, u16 bits)
{
RTL_W16(tp, IntrStatus, bits);
- mmiowb();
}
static void rtl_irq_disable(struct rtl8169_private *tp)
{
RTL_W16(tp, IntrMask, 0);
- mmiowb();
}
#define RTL_EVENT_NAPI_RX (RxOK | RxErr)
@@ -6127,10 +6125,8 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff
*skb,
if (unlikely(stop_queue))
netif_stop_queue(dev);
- if (__netdev_sent_queue(dev, skb->len, skb->xmit_more)) {
+ if (__netdev_sent_queue(dev, skb->len, skb->xmit_more))
RTL_W8(tp, TxPoll, NPQ);
- mmiowb();
- }
if (unlikely(stop_queue)) {
/* Sync with rtl_tx:
@@ -6481,9 +6477,7 @@ static int rtl8169_poll(struct napi_struct *napi, int
budget)
if (work_done < budget) {
napi_complete_done(napi, work_done);
-
rtl_irq_enable(tp);
- mmiowb();
}
return work_done;
--
2.19.2
Re: [PATCH net-next] net: phy: fix two issues with linkmode bitmaps
On 25.11.2018 17:45, Andrew Lunn wrote:
> On Sun, Nov 25, 2018 at 03:23:42PM +0100, Heiner Kallweit wrote:
>> I wondered why ethtool suddenly reports that link partner doesn't
>> support aneg and GBit modes. It turned out that this is caused by two
>> bugs in conversion to linkmode bitmaps.
>>
>> 1. In genphy_read_status the value of phydev->lp_advertising is
>>overwritten, thus GBit modes aren't reported any longer.
>> 2. In mii_lpa_to_linkmode_lpa_t the aneg bit was overwritten by the
>>call to mii_adv_to_linkmode_adv_t.
>
> Hi Heiner
>
> Thanks for looking into this.
>
> There are more bugs :-(
>
> static inline void mii_lpa_to_linkmode_lpa_t(unsigned long *lp_advertising,
> u32 lpa)
> {
> if (lpa & LPA_LPACK)
> linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> lp_advertising);
>
> mii_adv_to_linkmode_adv_t(lp_advertising, lpa);
> }
>
> But
>
> static inline void mii_adv_to_linkmode_adv_t(unsigned long *advertising,
> u32 adv)
> {
> linkmode_zero(advertising);
>
> if (adv & ADVERTISE_10HALF)
> linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
> advertising);
>
> So the Autoneg_BIT gets cleared.
>
> I think the better fix is to take the linkmode_zero() out from here.
>
> Then:
>
> if (adv & ADVERTISE_10HALF)
>linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
> advertising);
> + else
> + linkmode_clear_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
> + advertising);
>
> for all the bits mii_adv_to_linkmode_adv_t() looks at.
>
> So mii_adv_to_linkmode_adv_t() only modifies bits it is responsible
> for, and leaves the others alone.
>
> Andrew
>
mii_adv_to_linkmode_adv_t() is used also in phy_mii_ioctl(), and I'm
not sure the proposed change is safe there.
Eventually we'd have three types of mii_xxx_to_linkmode_yyy functions:
1. Function first zeroes the destination linkmode bitmap
2. Function sets bits in the linkmode bitmap but doesn't clear bits
if condition isn't met
3. Function clears / sets bits it's responsible for
example case 1: mmd_eee_adv_to_linkmode
example case 2: mii_stat1000_to_linkmode_lpa_t
example case 3: what you just proposed as fix for
mii_adv_to_linkmode_adv_t
Because function naming is the same I'm afraid they easily can be used
incorrectly (the bugs we just discuss are good examples). Maybe it
could be an option to reflect the semantics in the name like this
(better suited proposals welcome):
case 1: mii_xxx_to_linkmode_yyy
case 2: mii_xxx_or_linkmode_yyy
case 3: mii_xxx_mod_linkmode_yyy
Heiner
[PATCH net-next] net: phy: fix two issues with linkmode bitmaps
I wondered why ethtool suddenly reports that link partner doesn't
support aneg and GBit modes. It turned out that this is caused by two
bugs in conversion to linkmode bitmaps.
1. In genphy_read_status the value of phydev->lp_advertising is
overwritten, thus GBit modes aren't reported any longer.
2. In mii_lpa_to_linkmode_lpa_t the aneg bit was overwritten by the
call to mii_adv_to_linkmode_adv_t.
Fixes: c0ec3c273677 ("net: phy: Convert u32 phydev->lp_advertising to linkmode")
Signed-off-by: Heiner Kallweit
---
drivers/net/phy/phy_device.c | 5 -
include/linux/mii.h | 4 ++--
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 0904002b1..94f60c08b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1696,6 +1696,7 @@ int genphy_read_status(struct phy_device *phydev)
int lpagb = 0;
int common_adv;
int common_adv_gb = 0;
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(lpa_tmp);
/* Update the link, but return if there was an error */
err = genphy_update_link(phydev);
@@ -1734,7 +1735,9 @@ int genphy_read_status(struct phy_device *phydev)
if (lpa < 0)
return lpa;
- mii_lpa_to_linkmode_lpa_t(phydev->lp_advertising, lpa);
+ mii_lpa_to_linkmode_lpa_t(lpa_tmp, lpa);
+ linkmode_or(phydev->lp_advertising, phydev->lp_advertising,
+ lpa_tmp);
adv = phy_read(phydev, MII_ADVERTISE);
if (adv < 0)
diff --git a/include/linux/mii.h b/include/linux/mii.h
index fb7ae4ae8..08450609d 100644
--- a/include/linux/mii.h
+++ b/include/linux/mii.h
@@ -413,11 +413,11 @@ static inline void mii_adv_to_linkmode_adv_t(unsigned
long *advertising,
static inline void mii_lpa_to_linkmode_lpa_t(unsigned long *lp_advertising,
u32 lpa)
{
+ mii_adv_to_linkmode_adv_t(lp_advertising, lpa);
+
if (lpa & LPA_LPACK)
linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
lp_advertising);
-
- mii_adv_to_linkmode_adv_t(lp_advertising, lpa);
}
/**
--
2.19.2
[PATCH net-next v2 2/2] r8169: make use of xmit_more and __netdev_sent_queue
Make use of xmit_more and add the functionality introduced with
3e59020abf0f ("net: bql: add __netdev_tx_sent_queue()").
I used the mlx4 driver as template.
Signed-off-by: Heiner Kallweit
---
v2:
- fix minor style issue
---
drivers/net/ethernet/realtek/r8169.c | 19 +--
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index 5ee684f9e..4114c2712 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -6069,6 +6069,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
struct device *d = tp_to_dev(tp);
dma_addr_t mapping;
u32 opts[2], len;
+ bool stop_queue;
int frags;
if (unlikely(!rtl_tx_slots_avail(tp, skb_shinfo(skb)->nr_frags))) {
@@ -6110,8 +6111,6 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
txd->opts2 = cpu_to_le32(opts[1]);
- netdev_sent_queue(dev, skb->len);
-
skb_tx_timestamp(skb);
/* Force memory writes to complete before releasing descriptor */
@@ -6124,16 +6123,16 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff
*skb,
tp->cur_tx += frags + 1;
- RTL_W8(tp, TxPoll, NPQ);
+ stop_queue = !rtl_tx_slots_avail(tp, MAX_SKB_FRAGS);
+ if (unlikely(stop_queue))
+ netif_stop_queue(dev);
- mmiowb();
+ if (__netdev_sent_queue(dev, skb->len, skb->xmit_more)) {
+ RTL_W8(tp, TxPoll, NPQ);
+ mmiowb();
+ }
- if (!rtl_tx_slots_avail(tp, MAX_SKB_FRAGS)) {
- /* Avoid wrongly optimistic queue wake-up: rtl_tx thread must
-* not miss a ring update when it notices a stopped queue.
-*/
- smp_wmb();
- netif_stop_queue(dev);
+ if (unlikely(stop_queue)) {
/* Sync with rtl_tx:
* - publish queue status and cur_tx ring index (write barrier)
* - refresh dirty_tx ring index (read barrier).
--
2.19.2
[PATCH net-next v2 1/2] net: core: add __netdev_sent_queue as variant of __netdev_tx_sent_queue
Similar to netdev_sent_queue add helper __netdev_sent_queue as variant
of __netdev_tx_sent_queue.
Signed-off-by: Heiner Kallweit
---
v2:
- no changes
---
include/linux/netdevice.h | 8
1 file changed, 8 insertions(+)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 1dcc0628b..a417fa501 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3214,6 +3214,14 @@ static inline void netdev_sent_queue(struct net_device
*dev, unsigned int bytes)
netdev_tx_sent_queue(netdev_get_tx_queue(dev, 0), bytes);
}
+static inline bool __netdev_sent_queue(struct net_device *dev,
+ unsigned int bytes,
+ bool xmit_more)
+{
+ return __netdev_tx_sent_queue(netdev_get_tx_queue(dev, 0), bytes,
+ xmit_more);
+}
+
static inline void netdev_tx_completed_queue(struct netdev_queue *dev_queue,
unsigned int pkts, unsigned int
bytes)
{
--
2.19.1
[PATCH net-next v2 0/2] r8169: make use of xmit_more and __netdev_sent_queue
This series adds helper __netdev_sent_queue to the core and makes use of it in the r8169 driver. Heiner Kallweit (2): net: core: add __netdev_sent_queue as variant of __netdev_tx_sent_queue r8169: make use of xmit_more and __netdev_sent_queue v2: - fix minor style issue drivers/net/ethernet/realtek/r8169.c | 19 +-- include/linux/netdevice.h| 8 2 files changed, 17 insertions(+), 10 deletions(-) -- 2.19.1
[PATCH net-next 1/2] net: core: add __netdev_sent_queue as variant of __netdev_tx_sent_queue
Similar to netdev_sent_queue add helper __netdev_sent_queue as variant
of __netdev_tx_sent_queue.
Signed-off-by: Heiner Kallweit
---
include/linux/netdevice.h | 8
1 file changed, 8 insertions(+)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 1dcc0628b..a417fa501 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3214,6 +3214,14 @@ static inline void netdev_sent_queue(struct net_device
*dev, unsigned int bytes)
netdev_tx_sent_queue(netdev_get_tx_queue(dev, 0), bytes);
}
+static inline bool __netdev_sent_queue(struct net_device *dev,
+ unsigned int bytes,
+ bool xmit_more)
+{
+ return __netdev_tx_sent_queue(netdev_get_tx_queue(dev, 0), bytes,
+ xmit_more);
+}
+
static inline void netdev_tx_completed_queue(struct netdev_queue *dev_queue,
unsigned int pkts, unsigned int
bytes)
{
--
2.19.1
[PATCH net-next 2/2] r8169: make use of xmit_more and __netdev_sent_queue
Make use of xmit_more and add the functionality introduced with
3e59020abf0f ("net: bql: add __netdev_tx_sent_queue()").
I used the mlx4 driver as template.
Signed-off-by: Heiner Kallweit
---
drivers/net/ethernet/realtek/r8169.c | 19 +--
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index 5ee684f9e..7a979c1e5 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -6070,6 +6070,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
dma_addr_t mapping;
u32 opts[2], len;
int frags;
+ bool stop_queue;
if (unlikely(!rtl_tx_slots_avail(tp, skb_shinfo(skb)->nr_frags))) {
netif_err(tp, drv, dev, "BUG! Tx Ring full when queue
awake!\n");
@@ -6110,8 +6111,6 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
txd->opts2 = cpu_to_le32(opts[1]);
- netdev_sent_queue(dev, skb->len);
-
skb_tx_timestamp(skb);
/* Force memory writes to complete before releasing descriptor */
@@ -6124,16 +6123,16 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff
*skb,
tp->cur_tx += frags + 1;
- RTL_W8(tp, TxPoll, NPQ);
+ stop_queue = !rtl_tx_slots_avail(tp, MAX_SKB_FRAGS);
+ if (unlikely(stop_queue))
+ netif_stop_queue(dev);
- mmiowb();
+ if (__netdev_sent_queue(dev, skb->len, skb->xmit_more)) {
+ RTL_W8(tp, TxPoll, NPQ);
+ mmiowb();
+ }
- if (!rtl_tx_slots_avail(tp, MAX_SKB_FRAGS)) {
- /* Avoid wrongly optimistic queue wake-up: rtl_tx thread must
-* not miss a ring update when it notices a stopped queue.
-*/
- smp_wmb();
- netif_stop_queue(dev);
+ if (unlikely(stop_queue)) {
/* Sync with rtl_tx:
* - publish queue status and cur_tx ring index (write barrier)
* - refresh dirty_tx ring index (read barrier).
--
2.19.1
[PATCH net-next 0/2] r8169: make use of xmit_more and __netdev_sent_queue
This series adds helper __netdev_sent_queue to the core and makes use of it in the r8169 driver. Heiner Kallweit (2): net: core: add __netdev_sent_queue as variant of __netdev_tx_sent_queue r8169: make use of xmit_more and __netdev_sent_queue drivers/net/ethernet/realtek/r8169.c | 19 +-- include/linux/netdevice.h| 8 2 files changed, 17 insertions(+), 10 deletions(-) -- 2.19.1
[PATCH net] net: phy: add workaround for issue where PHY driver doesn't bind to the device
After switching the r8169 driver to use phylib some user reported that
their network is broken. This was caused by the genphy PHY driver being
used instead of the dedicated PHY driver for the RTL8211B. Users
reported that loading the Realtek PHY driver module upfront fixes the
issue. See also this mail thread:
https://marc.info/?t=15427978183=1=2
The issue is quite weird and the root cause seems to be somewhere in
the base driver core. The patch works around the issue and may be
removed once the actual issue is fixed.
The Fixes tag refers to the first reported occurrence of the issue.
The issue itself may have been existing much longer and it may affect
users of other network chips as well. Users typically will recognize
this issue only if their PHY stops working when being used with the
genphy driver.
Fixes: f1e911d5d0df ("r8169: add basic phylib support")
Signed-off-by: Heiner Kallweit
---
I'm not sure how long it will take to find and fix the root cause of
the issue. With this workaround affected users have a working network
again from 4.19.5 at least.
---
drivers/net/phy/phy_device.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index e06613f2d..0904002b1 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2255,6 +2255,14 @@ int phy_driver_register(struct phy_driver *new_driver,
struct module *owner)
new_driver->mdiodrv.driver.remove = phy_remove;
new_driver->mdiodrv.driver.owner = owner;
+ /* The following works around an issue where the PHY driver doesn't bind
+* to the device, resulting in the genphy driver being used instead of
+* the dedicated driver. The root cause of the issue isn't known yet
+* and seems to be in the base driver core. Once this is fixed we may
+* remove this workaround.
+*/
+ new_driver->mdiodrv.driver.probe_type = PROBE_FORCE_SYNCHRONOUS;
+
retval = driver_register(_driver->mdiodrv.driver);
if (retval) {
pr_err("%s: Error %d in registering driver\n",
--
2.19.1
[PATCH net-next 3/5] r8169: simplify detecting chip versions with same XID
For the GMII chip versions we set the version number which was set
already. This can be simplified.
Signed-off-by: Heiner Kallweit
---
drivers/net/ethernet/realtek/r8169.c | 19 +++
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index 1e549b26b..9a696455e 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -2116,18 +2116,13 @@ static void rtl8169_get_mac_version(struct
rtl8169_private *tp)
if (tp->mac_version == RTL_GIGA_MAC_NONE) {
dev_err(tp_to_dev(tp), "unknown chip XID %03x\n", reg & 0xfcf);
- } else if (tp->mac_version == RTL_GIGA_MAC_VER_42) {
- tp->mac_version = tp->supports_gmii ?
- RTL_GIGA_MAC_VER_42 :
- RTL_GIGA_MAC_VER_43;
- } else if (tp->mac_version == RTL_GIGA_MAC_VER_45) {
- tp->mac_version = tp->supports_gmii ?
- RTL_GIGA_MAC_VER_45 :
- RTL_GIGA_MAC_VER_47;
- } else if (tp->mac_version == RTL_GIGA_MAC_VER_46) {
- tp->mac_version = tp->supports_gmii ?
- RTL_GIGA_MAC_VER_46 :
- RTL_GIGA_MAC_VER_48;
+ } else if (!tp->supports_gmii) {
+ if (tp->mac_version == RTL_GIGA_MAC_VER_42)
+ tp->mac_version = RTL_GIGA_MAC_VER_43;
+ else if (tp->mac_version == RTL_GIGA_MAC_VER_45)
+ tp->mac_version = RTL_GIGA_MAC_VER_47;
+ else if (tp->mac_version == RTL_GIGA_MAC_VER_46)
+ tp->mac_version = RTL_GIGA_MAC_VER_48;
}
}
--
2.19.1
[PATCH net-next 4/5] r8169: use napi_consume_skb where possible
Use napi_consume_skb() where possible to profit from
bulk free infrastructure.
Signed-off-by: Heiner Kallweit
---
drivers/net/ethernet/realtek/r8169.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index 9a696455e..2ca0b2ed9 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -6204,7 +6204,8 @@ static void rtl8169_pcierr_interrupt(struct net_device
*dev)
rtl_schedule_task(tp, RTL_FLAG_TASK_RESET_PENDING);
}
-static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
+static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp,
+ int budget)
{
unsigned int dirty_tx, tx_left, bytes_compl = 0, pkts_compl = 0;
@@ -6232,7 +6233,7 @@ static void rtl_tx(struct net_device *dev, struct
rtl8169_private *tp)
if (status & LastFrag) {
pkts_compl++;
bytes_compl += tx_skb->skb->len;
- dev_consume_skb_any(tx_skb->skb);
+ napi_consume_skb(tx_skb->skb, budget);
tx_skb->skb = NULL;
}
dirty_tx++;
@@ -6475,7 +6476,7 @@ static int rtl8169_poll(struct napi_struct *napi, int
budget)
work_done = rtl_rx(dev, tp, (u32) budget);
- rtl_tx(dev, tp);
+ rtl_tx(dev, tp, budget);
if (work_done < budget) {
napi_complete_done(napi, work_done);
--
2.19.1
[PATCH net-next 5/5] r8169: replace macro TX_FRAGS_READY_FOR with a function
Replace macro TX_FRAGS_READY_FOR with function rtl_tx_slots_avail
to make code cleaner and type-safe.
Signed-off-by: Heiner Kallweit
---
drivers/net/ethernet/realtek/r8169.c | 24 +---
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index 2ca0b2ed9..f768b966e 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -56,13 +56,6 @@
#define R8169_MSG_DEFAULT \
(NETIF_MSG_DRV | NETIF_MSG_PROBE | NETIF_MSG_IFUP | NETIF_MSG_IFDOWN)
-#define TX_SLOTS_AVAIL(tp) \
- (tp->dirty_tx + NUM_TX_DESC - tp->cur_tx)
-
-/* A skbuff with nr_frags needs nr_frags+1 entries in the tx queue */
-#define TX_FRAGS_READY_FOR(tp,nr_frags) \
- (TX_SLOTS_AVAIL(tp) >= (nr_frags + 1))
-
/* Maximum number of multicast addresses to filter (vs. Rx-all-multicast).
The RTL chips use a 64 element hash table based on the Ethernet CRC. */
static const int multicast_filter_limit = 32;
@@ -6058,6 +6051,15 @@ static bool rtl8169_tso_csum_v2(struct rtl8169_private
*tp,
return true;
}
+static bool rtl_tx_slots_avail(struct rtl8169_private *tp,
+ unsigned int nr_frags)
+{
+ unsigned int slots_avail = tp->dirty_tx + NUM_TX_DESC - tp->cur_tx;
+
+ /* A skbuff with nr_frags needs nr_frags+1 entries in the tx queue */
+ return slots_avail > nr_frags;
+}
+
static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
struct net_device *dev)
{
@@ -6069,7 +6071,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
u32 opts[2], len;
int frags;
- if (unlikely(!TX_FRAGS_READY_FOR(tp, skb_shinfo(skb)->nr_frags))) {
+ if (unlikely(!rtl_tx_slots_avail(tp, skb_shinfo(skb)->nr_frags))) {
netif_err(tp, drv, dev, "BUG! Tx Ring full when queue
awake!\n");
goto err_stop_0;
}
@@ -6126,7 +6128,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
mmiowb();
- if (!TX_FRAGS_READY_FOR(tp, MAX_SKB_FRAGS)) {
+ if (!rtl_tx_slots_avail(tp, MAX_SKB_FRAGS)) {
/* Avoid wrongly optimistic queue wake-up: rtl_tx thread must
* not miss a ring update when it notices a stopped queue.
*/
@@ -6140,7 +6142,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
* can't.
*/
smp_mb();
- if (TX_FRAGS_READY_FOR(tp, MAX_SKB_FRAGS))
+ if (rtl_tx_slots_avail(tp, MAX_SKB_FRAGS))
netif_wake_queue(dev);
}
@@ -6258,7 +6260,7 @@ static void rtl_tx(struct net_device *dev, struct
rtl8169_private *tp,
*/
smp_mb();
if (netif_queue_stopped(dev) &&
- TX_FRAGS_READY_FOR(tp, MAX_SKB_FRAGS)) {
+ rtl_tx_slots_avail(tp, MAX_SKB_FRAGS)) {
netif_wake_queue(dev);
}
/*
--
2.19.1
[PATCH net-next 2/5] r8169: remove default chip versions
Even the chip versions within a family have so many differences that
using a default chip version doesn't really make sense. Instead
of leaving a best case flaky network connectivity, bail out and
report the unknown chip version.
Signed-off-by: Heiner Kallweit
---
drivers/net/ethernet/realtek/r8169.c | 15 +--
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index bef89ba50..1e549b26b 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -2011,8 +2011,7 @@ static const struct ethtool_ops rtl8169_ethtool_ops = {
.set_link_ksettings = phy_ethtool_set_link_ksettings,
};
-static void rtl8169_get_mac_version(struct rtl8169_private *tp,
- u8 default_version)
+static void rtl8169_get_mac_version(struct rtl8169_private *tp)
{
/*
* The driver currently handles the 8168Bf and the 8168Be identically
@@ -2116,9 +2115,7 @@ static void rtl8169_get_mac_version(struct
rtl8169_private *tp,
tp->mac_version = p->mac_version;
if (tp->mac_version == RTL_GIGA_MAC_NONE) {
- dev_notice(tp_to_dev(tp),
- "unknown MAC, using family default\n");
- tp->mac_version = default_version;
+ dev_err(tp_to_dev(tp), "unknown chip XID %03x\n", reg & 0xfcf);
} else if (tp->mac_version == RTL_GIGA_MAC_VER_42) {
tp->mac_version = tp->supports_gmii ?
RTL_GIGA_MAC_VER_42 :
@@ -6976,27 +6973,23 @@ static const struct rtl_cfg_info {
u16 irq_mask;
unsigned int has_gmii:1;
const struct rtl_coalesce_info *coalesce_info;
- u8 default_ver;
} rtl_cfg_infos [] = {
[RTL_CFG_0] = {
.hw_start = rtl_hw_start_8169,
.irq_mask = SYSErr | LinkChg | RxOverflow | RxFIFOOver,
.has_gmii = 1,
.coalesce_info = rtl_coalesce_info_8169,
- .default_ver= RTL_GIGA_MAC_VER_01,
},
[RTL_CFG_1] = {
.hw_start = rtl_hw_start_8168,
.irq_mask = LinkChg | RxOverflow,
.has_gmii = 1,
.coalesce_info = rtl_coalesce_info_8168_8136,
- .default_ver= RTL_GIGA_MAC_VER_11,
},
[RTL_CFG_2] = {
.hw_start = rtl_hw_start_8101,
.irq_mask = LinkChg | RxOverflow | RxFIFOOver,
.coalesce_info = rtl_coalesce_info_8168_8136,
- .default_ver= RTL_GIGA_MAC_VER_13,
}
};
@@ -7259,7 +7252,9 @@ static int rtl_init_one(struct pci_dev *pdev, const
struct pci_device_id *ent)
tp->mmio_addr = pcim_iomap_table(pdev)[region];
/* Identify chip attached to board */
- rtl8169_get_mac_version(tp, cfg->default_ver);
+ rtl8169_get_mac_version(tp);
+ if (tp->mac_version == RTL_GIGA_MAC_NONE)
+ return -ENODEV;
if (rtl_tbi_enabled(tp)) {
dev_err(>dev, "TBI fiber mode not supported\n");
--
2.19.1
[PATCH net-next 1/5] r8169: remove ancient GCC bug workaround in a second place
Remove ancient GCC bug workaround in a second place and factor out
rtl_8169_get_txd_opts1.
Signed-off-by: Heiner Kallweit
---
drivers/net/ethernet/realtek/r8169.c | 25 ++---
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index f5781285a..bef89ba50 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -5840,6 +5840,16 @@ static void rtl8169_tx_timeout(struct net_device *dev)
rtl_schedule_task(tp, RTL_FLAG_TASK_RESET_PENDING);
}
+static __le32 rtl8169_get_txd_opts1(u32 opts0, u32 len, unsigned int entry)
+{
+ u32 status = opts0 | len;
+
+ if (entry == NUM_TX_DESC - 1)
+ status |= RingEnd;
+
+ return cpu_to_le32(status);
+}
+
static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
u32 *opts)
{
@@ -5852,7 +5862,7 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp,
struct sk_buff *skb,
for (cur_frag = 0; cur_frag < info->nr_frags; cur_frag++) {
const skb_frag_t *frag = info->frags + cur_frag;
dma_addr_t mapping;
- u32 status, len;
+ u32 len;
void *addr;
entry = (entry + 1) % NUM_TX_DESC;
@@ -5868,11 +5878,7 @@ static int rtl8169_xmit_frags(struct rtl8169_private
*tp, struct sk_buff *skb,
goto err_out;
}
- status = opts[0] | len;
- if (entry == NUM_TX_DESC - 1)
- status |= RingEnd;
-
- txd->opts1 = cpu_to_le32(status);
+ txd->opts1 = rtl8169_get_txd_opts1(opts[0], len, entry);
txd->opts2 = cpu_to_le32(opts[1]);
txd->addr = cpu_to_le64(mapping);
@@ -6068,8 +6074,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
struct TxDesc *txd = tp->TxDescArray + entry;
struct device *d = tp_to_dev(tp);
dma_addr_t mapping;
- u32 status, len;
- u32 opts[2];
+ u32 opts[2], len;
int frags;
if (unlikely(!TX_FRAGS_READY_FOR(tp, skb_shinfo(skb)->nr_frags))) {
@@ -6118,9 +6123,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
/* Force memory writes to complete before releasing descriptor */
dma_wmb();
- /* Anti gcc 2.95.3 bugware (sic) */
- status = opts[0] | len | (RingEnd * !((entry + 1) % NUM_TX_DESC));
- txd->opts1 = cpu_to_le32(status);
+ txd->opts1 = rtl8169_get_txd_opts1(opts[0], len, entry);
/* Force all memory writes to complete before notifying device */
wmb();
--
2.19.1
[PATCH net-next 0/5] r8169: some functional improvements
This series includes a few functional improvements. Heiner Kallweit (5): r8169: Remove ancient GCC bug workaround in a second place r8169: remove default chip versions r8169: simplify detecting chip versions with same XID r8169: use napi_consume_skb where possible r8169: replace macro TX_FRAGS_READY_FOR with a function drivers/net/ethernet/realtek/r8169.c | 90 +--- 1 file changed, 43 insertions(+), 47 deletions(-) -- 2.19.1
[PATCH net] MAINTAINERS: add myself as co-maintainer for r8169
Meanwhile I know the driver quite well and I refactored bigger parts of it. As a result people contact me already with r8169 questions. Therefore I'd volunteer to become co-maintainer of the driver also officially. Signed-off-by: Heiner Kallweit --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 2988ecbf6..60a96974f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -180,6 +180,7 @@ F: drivers/net/hamradio/6pack.c 8169 10/100/1000 GIGABIT ETHERNET DRIVER M: Realtek linux nic maintainers +M: Heiner Kallweit L: netdev@vger.kernel.org S: Maintained F: drivers/net/ethernet/realtek/r8169.c -- 2.19.1
[PATCH net-next 05/11] r8169: use PCI_VDEVICE macro
Using macro PCI_VDEVICE helps to simplify the PCI ID table.
Signed-off-by: Heiner Kallweit
---
drivers/net/ethernet/realtek/r8169.c | 28 ++--
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index 81bdd2fe0..ee6458070 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -212,24 +212,24 @@ enum cfg_version {
};
static const struct pci_device_id rtl8169_pci_tbl[] = {
- { PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0x8129), 0, 0, RTL_CFG_0 },
- { PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0x8136), 0, 0, RTL_CFG_2 },
- { PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0x8161), 0, 0, RTL_CFG_1 },
- { PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0x8167), 0, 0, RTL_CFG_0 },
- { PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0x8168), 0, 0, RTL_CFG_1 },
- { PCI_DEVICE(PCI_VENDOR_ID_NCUBE, 0x8168), 0, 0, RTL_CFG_1 },
- { PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0x8169), 0, 0, RTL_CFG_0 },
- { PCI_VENDOR_ID_DLINK, 0x4300,
- PCI_VENDOR_ID_DLINK, 0x4b10, 0, 0, RTL_CFG_1 },
- { PCI_DEVICE(PCI_VENDOR_ID_DLINK, 0x4300), 0, 0, RTL_CFG_0 },
- { PCI_DEVICE(PCI_VENDOR_ID_DLINK, 0x4302), 0, 0, RTL_CFG_0 },
- { PCI_DEVICE(PCI_VENDOR_ID_AT, 0xc107), 0, 0, RTL_CFG_0 },
- { PCI_DEVICE(PCI_VENDOR_ID_USR, 0x0116), 0, 0, RTL_CFG_0 },
+ { PCI_VDEVICE(REALTEK, 0x8129), RTL_CFG_0 },
+ { PCI_VDEVICE(REALTEK, 0x8136), RTL_CFG_2 },
+ { PCI_VDEVICE(REALTEK, 0x8161), RTL_CFG_1 },
+ { PCI_VDEVICE(REALTEK, 0x8167), RTL_CFG_0 },
+ { PCI_VDEVICE(REALTEK, 0x8168), RTL_CFG_1 },
+ { PCI_VDEVICE(NCUBE,0x8168), RTL_CFG_1 },
+ { PCI_VDEVICE(REALTEK, 0x8169), RTL_CFG_0 },
+ { PCI_VENDOR_ID_DLINK, 0x4300,
+ PCI_VENDOR_ID_DLINK, 0x4b10, 0, 0, RTL_CFG_1 },
+ { PCI_VDEVICE(DLINK,0x4300), RTL_CFG_0 },
+ { PCI_VDEVICE(DLINK,0x4302), RTL_CFG_0 },
+ { PCI_VDEVICE(AT, 0xc107), RTL_CFG_0 },
+ { PCI_VDEVICE(USR, 0x0116), RTL_CFG_0 },
{ PCI_VENDOR_ID_LINKSYS,0x1032,
PCI_ANY_ID, 0x0024, 0, 0, RTL_CFG_0 },
{ 0x0001, 0x8168,
PCI_ANY_ID, 0x2410, 0, 0, RTL_CFG_2 },
- {0,},
+ {}
};
MODULE_DEVICE_TABLE(pci, rtl8169_pci_tbl);
--
2.19.1
[PATCH net-next 03/11] r8169: remove unused interrupt sources
Setting PCSTimeout interrupt source was copied from the vendor driver
which uses the chip programmable timer interrupt. The mainline driver
doesn't use this timer interrupt.
SYSErr indicates a PCI error and isn't defined on the PCIe models.
Signed-off-by: Heiner Kallweit
---
drivers/net/ethernet/realtek/r8169.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index fa6349b5d..9e04566a2 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -5389,7 +5389,7 @@ static void rtl_hw_start_8168(struct rtl8169_private *tp)
/* Work around for RxFIFO overflow. */
if (tp->mac_version == RTL_GIGA_MAC_VER_11) {
- tp->event_slow |= RxFIFOOver | PCSTimeout;
+ tp->event_slow |= RxFIFOOver;
tp->event_slow &= ~RxOverflow;
}
@@ -7027,15 +7027,14 @@ static const struct rtl_cfg_info {
},
[RTL_CFG_1] = {
.hw_start = rtl_hw_start_8168,
- .event_slow = SYSErr | LinkChg | RxOverflow,
+ .event_slow = LinkChg | RxOverflow,
.has_gmii = 1,
.coalesce_info = rtl_coalesce_info_8168_8136,
.default_ver= RTL_GIGA_MAC_VER_11,
},
[RTL_CFG_2] = {
.hw_start = rtl_hw_start_8101,
- .event_slow = SYSErr | LinkChg | RxOverflow | RxFIFOOver |
- PCSTimeout,
+ .event_slow = LinkChg | RxOverflow | RxFIFOOver,
.coalesce_info = rtl_coalesce_info_8168_8136,
.default_ver= RTL_GIGA_MAC_VER_13,
}
--
2.19.1
[PATCH net-next 06/11] r8169: remove print_mac_version
The syslog message printed on driver load allows to easily identify
the mac version number (based on chip name and XID). So we don't
need this extra debug message which is wrong anyway because e.g.
RTL_GIGA_MAC_VER_01 has value 0.
Signed-off-by: Heiner Kallweit
---
drivers/net/ethernet/realtek/r8169.c | 9 -
1 file changed, 9 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index ee6458070..a6b12adbc 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -2170,11 +2170,6 @@ static void rtl8169_get_mac_version(struct
rtl8169_private *tp,
}
}
-static void rtl8169_print_mac_version(struct rtl8169_private *tp)
-{
- netif_dbg(tp, drv, tp->dev, "mac_version = 0x%02x\n", tp->mac_version);
-}
-
struct phy_reg {
u16 reg;
u16 val;
@@ -3897,8 +3892,6 @@ static void rtl_hw_phy_config(struct net_device *dev)
{
struct rtl8169_private *tp = netdev_priv(dev);
- rtl8169_print_mac_version(tp);
-
switch (tp->mac_version) {
case RTL_GIGA_MAC_VER_01:
break;
@@ -7340,8 +7333,6 @@ static int rtl_init_one(struct pci_dev *pdev, const
struct pci_device_id *ent)
rtl_init_mdio_ops(tp);
rtl_init_jumbo_ops(tp);
- rtl8169_print_mac_version(tp);
-
chipset = tp->mac_version;
rc = rtl_alloc_irq(tp);
--
2.19.1
[PATCH net-next 04/11] r8169: replace event_slow with irq_mask
Recently the "slow event" handler was removed, therefore the member
name isn't appropriate any longer. In addition store the full mask,
including the RTL_EVENT_NAPI interrupt source bits.
Signed-off-by: Heiner Kallweit
---
drivers/net/ethernet/realtek/r8169.c | 22 +++---
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index 9e04566a2..81bdd2fe0 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -661,7 +661,7 @@ struct rtl8169_private {
struct ring_info tx_skb[NUM_TX_DESC]; /* Tx data buffers */
u16 cp_cmd;
- u16 event_slow;
+ u16 irq_mask;
const struct rtl_coalesce_info *coalesce_info;
struct clk *clk;
@@ -1340,7 +1340,7 @@ static void rtl_irq_disable(struct rtl8169_private *tp)
static void rtl_irq_enable(struct rtl8169_private *tp)
{
- RTL_W16(tp, IntrMask, RTL_EVENT_NAPI | tp->event_slow);
+ RTL_W16(tp, IntrMask, tp->irq_mask);
}
static void rtl8169_irq_mask_and_ack(struct rtl8169_private *tp)
@@ -5389,8 +5389,8 @@ static void rtl_hw_start_8168(struct rtl8169_private *tp)
/* Work around for RxFIFO overflow. */
if (tp->mac_version == RTL_GIGA_MAC_VER_11) {
- tp->event_slow |= RxFIFOOver;
- tp->event_slow &= ~RxOverflow;
+ tp->irq_mask |= RxFIFOOver;
+ tp->irq_mask &= ~RxOverflow;
}
switch (tp->mac_version) {
@@ -5627,7 +5627,7 @@ static void rtl_hw_start_8106(struct rtl8169_private *tp)
static void rtl_hw_start_8101(struct rtl8169_private *tp)
{
if (tp->mac_version >= RTL_GIGA_MAC_VER_30)
- tp->event_slow &= ~RxFIFOOver;
+ tp->irq_mask &= ~RxFIFOOver;
if (tp->mac_version == RTL_GIGA_MAC_VER_13 ||
tp->mac_version == RTL_GIGA_MAC_VER_16)
@@ -6456,7 +6456,7 @@ static irqreturn_t rtl8169_interrupt(int irq, void
*dev_instance)
struct rtl8169_private *tp = dev_instance;
u16 status = rtl_get_events(tp);
- if (status == 0x || !(status & (RTL_EVENT_NAPI | tp->event_slow)))
+ if (status == 0x || !(status & tp->irq_mask))
return IRQ_NONE;
if (unlikely(status & SYSErr)) {
@@ -7013,28 +7013,28 @@ static const struct net_device_ops rtl_netdev_ops = {
static const struct rtl_cfg_info {
void (*hw_start)(struct rtl8169_private *tp);
- u16 event_slow;
+ u16 irq_mask;
unsigned int has_gmii:1;
const struct rtl_coalesce_info *coalesce_info;
u8 default_ver;
} rtl_cfg_infos [] = {
[RTL_CFG_0] = {
.hw_start = rtl_hw_start_8169,
- .event_slow = SYSErr | LinkChg | RxOverflow | RxFIFOOver,
+ .irq_mask = SYSErr | LinkChg | RxOverflow | RxFIFOOver,
.has_gmii = 1,
.coalesce_info = rtl_coalesce_info_8169,
.default_ver= RTL_GIGA_MAC_VER_01,
},
[RTL_CFG_1] = {
.hw_start = rtl_hw_start_8168,
- .event_slow = LinkChg | RxOverflow,
+ .irq_mask = LinkChg | RxOverflow,
.has_gmii = 1,
.coalesce_info = rtl_coalesce_info_8168_8136,
.default_ver= RTL_GIGA_MAC_VER_11,
},
[RTL_CFG_2] = {
.hw_start = rtl_hw_start_8101,
- .event_slow = LinkChg | RxOverflow | RxFIFOOver,
+ .irq_mask = LinkChg | RxOverflow | RxFIFOOver,
.coalesce_info = rtl_coalesce_info_8168_8136,
.default_ver= RTL_GIGA_MAC_VER_13,
}
@@ -7415,7 +7415,7 @@ static int rtl_init_one(struct pci_dev *pdev, const
struct pci_device_id *ent)
dev->max_mtu = jumbo_max;
tp->hw_start = cfg->hw_start;
- tp->event_slow = cfg->event_slow;
+ tp->irq_mask = RTL_EVENT_NAPI | cfg->irq_mask;
tp->coalesce_info = cfg->coalesce_info;
tp->rtl_fw = RTL_FIRMWARE_UNKNOWN;
--
2.19.1
[PATCH net-next 11/11] r8169: improve chip version identification
Only the upper 12 bits are used for chip identification, this helps
to reduce the size of array mac_info.
Signed-off-by: Heiner Kallweit
---
drivers/net/ethernet/realtek/r8169.c | 119 +--
1 file changed, 59 insertions(+), 60 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index 5e6bd1a5e..f5781285a 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -2026,92 +2026,91 @@ static void rtl8169_get_mac_version(struct
rtl8169_private *tp,
* (RTL_R32(tp, TxConfig) & 0x70) == 0x20 ? 8101Eb : 8101Ec
*/
static const struct rtl_mac_info {
- u32 mask;
- u32 val;
- int mac_version;
+ u16 mask;
+ u16 val;
+ u16 mac_version;
} mac_info[] = {
/* 8168EP family. */
- { 0x7cf0, 0x5020, RTL_GIGA_MAC_VER_51 },
- { 0x7cf0, 0x5010, RTL_GIGA_MAC_VER_50 },
- { 0x7cf0, 0x5000, RTL_GIGA_MAC_VER_49 },
+ { 0x7cf, 0x502, RTL_GIGA_MAC_VER_51 },
+ { 0x7cf, 0x501, RTL_GIGA_MAC_VER_50 },
+ { 0x7cf, 0x500, RTL_GIGA_MAC_VER_49 },
/* 8168H family. */
- { 0x7cf0, 0x5410, RTL_GIGA_MAC_VER_46 },
- { 0x7cf0, 0x5400, RTL_GIGA_MAC_VER_45 },
+ { 0x7cf, 0x541, RTL_GIGA_MAC_VER_46 },
+ { 0x7cf, 0x540, RTL_GIGA_MAC_VER_45 },
/* 8168G family. */
- { 0x7cf0, 0x5c80, RTL_GIGA_MAC_VER_44 },
- { 0x7cf0, 0x5090, RTL_GIGA_MAC_VER_42 },
- { 0x7cf0, 0x4c10, RTL_GIGA_MAC_VER_41 },
- { 0x7cf0, 0x4c00, RTL_GIGA_MAC_VER_40 },
+ { 0x7cf, 0x5c8, RTL_GIGA_MAC_VER_44 },
+ { 0x7cf, 0x509, RTL_GIGA_MAC_VER_42 },
+ { 0x7cf, 0x4c1, RTL_GIGA_MAC_VER_41 },
+ { 0x7cf, 0x4c0, RTL_GIGA_MAC_VER_40 },
/* 8168F family. */
- { 0x7c80, 0x4880, RTL_GIGA_MAC_VER_38 },
- { 0x7cf0, 0x4810, RTL_GIGA_MAC_VER_36 },
- { 0x7cf0, 0x4800, RTL_GIGA_MAC_VER_35 },
+ { 0x7c8, 0x488, RTL_GIGA_MAC_VER_38 },
+ { 0x7cf, 0x481, RTL_GIGA_MAC_VER_36 },
+ { 0x7cf, 0x480, RTL_GIGA_MAC_VER_35 },
/* 8168E family. */
- { 0x7c80, 0x2c80, RTL_GIGA_MAC_VER_34 },
- { 0x7cf0, 0x2c10, RTL_GIGA_MAC_VER_32 },
- { 0x7c80, 0x2c00, RTL_GIGA_MAC_VER_33 },
+ { 0x7c8, 0x2c8, RTL_GIGA_MAC_VER_34 },
+ { 0x7cf, 0x2c1, RTL_GIGA_MAC_VER_32 },
+ { 0x7c8, 0x2c0, RTL_GIGA_MAC_VER_33 },
/* 8168D family. */
- { 0x7cf0, 0x2810, RTL_GIGA_MAC_VER_25 },
- { 0x7c80, 0x2800, RTL_GIGA_MAC_VER_26 },
+ { 0x7cf, 0x281, RTL_GIGA_MAC_VER_25 },
+ { 0x7c8, 0x280, RTL_GIGA_MAC_VER_26 },
/* 8168DP family. */
- { 0x7cf0, 0x2880, RTL_GIGA_MAC_VER_27 },
- { 0x7cf0, 0x28a0, RTL_GIGA_MAC_VER_28 },
- { 0x7cf0, 0x28b0, RTL_GIGA_MAC_VER_31 },
+ { 0x7cf, 0x288, RTL_GIGA_MAC_VER_27 },
+ { 0x7cf, 0x28a, RTL_GIGA_MAC_VER_28 },
+ { 0x7cf, 0x28b, RTL_GIGA_MAC_VER_31 },
/* 8168C family. */
- { 0x7cf0, 0x3c90, RTL_GIGA_MAC_VER_23 },
- { 0x7cf0, 0x3c80, RTL_GIGA_MAC_VER_18 },
- { 0x7c80, 0x3c80, RTL_GIGA_MAC_VER_24 },
- { 0x7cf0, 0x3c00, RTL_GIGA_MAC_VER_19 },
- { 0x7cf0, 0x3c20, RTL_GIGA_MAC_VER_20 },
- { 0x7cf0, 0x3c30, RTL_GIGA_MAC_VER_21 },
- { 0x7c80, 0x3c00, RTL_GIGA_MAC_VER_22 },
+ { 0x7cf, 0x3c9, RTL_GIGA_MAC_VER_23 },
+ { 0x7cf, 0x3c8, RTL_GIGA_MAC_VER_18 },
+ { 0x7c8, 0x3c8, RTL_GIGA_MAC_VER_24 },
+ { 0x7cf, 0x3c0, RTL_GIGA_MAC_VER_19 },
+ { 0x7cf, 0x3c2, RTL_GIGA_MAC_VER_20 },
+ { 0x7cf, 0x3c3, RTL_GIGA_MAC_VER_21 },
+ { 0x7c8, 0x3c0, RTL_GIGA_MAC_VER_22 },
/* 8168B family. */
- { 0x7cf0, 0x3800, RTL_GIGA_MAC_VER_12 },
- { 0x7c80, 0x3800, RTL_GIGA_MAC_VER_17 },
- { 0x7c80, 0x3000, RTL_GIGA_MAC_VER_11 },
+ { 0x7cf, 0x380, RTL_GIGA_MAC_VER_12 },
+ { 0x7c8, 0x380, RTL_GIGA_MAC_VER_17 },
+ { 0x7c8, 0
[PATCH net-next 07/11] r8169: remove "not PCI Express" message
The ones who want to know can easily identify whether chip is PCI or PCIe based on the chip name. I doubt there's any benefit in this message, so remove it. Signed-off-by: Heiner Kallweit --- drivers/net/ethernet/realtek/r8169.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c index a6b12adbc..3ffd9c18b 100644 --- a/drivers/net/ethernet/realtek/r8169.c +++ b/drivers/net/ethernet/realtek/r8169.c @@ -7291,9 +7291,6 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) tp->mmio_addr = pcim_iomap_table(pdev)[region]; - if (!pci_is_pcie(pdev)) - dev_info(>dev, "not PCI Express\n"); - /* Identify chip attached to board */ rtl8169_get_mac_version(tp, cfg->default_ver); -- 2.19.1
[PATCH net-next 09/11] r8169: remove workaround for ancient gcc bug
The kernel can't be built any longer with this ancient GCC version. Eventually it becomes clear what this statement actually does. Signed-off-by: Heiner Kallweit --- drivers/net/ethernet/realtek/r8169.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c index 6736b804c..b5f9c3511 100644 --- a/drivers/net/ethernet/realtek/r8169.c +++ b/drivers/net/ethernet/realtek/r8169.c @@ -5903,9 +5903,9 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb, goto err_out; } - /* Anti gcc 2.95.3 bugware (sic) */ - status = opts[0] | len | - (RingEnd * !((entry + 1) % NUM_TX_DESC)); + status = opts[0] | len; + if (entry == NUM_TX_DESC - 1) + status |= RingEnd; txd->opts1 = cpu_to_le32(status); txd->opts2 = cpu_to_le32(opts[1]); -- 2.19.1
[PATCH net-next 10/11] r8169: simplify ocp functions
rtl8168_oob_notify is used in rtl8168dp_driver_start and
rtl8168dp_driver_stop only, so we can rename it to r8168dp_oob_notify.
The same applies to condition rtl_ocp_read_cond which can be renamed
to rtl_dp_ocp_read_cond. This allows to simplify the code.
Signed-off-by: Heiner Kallweit
---
drivers/net/ethernet/realtek/r8169.c | 68 +++-
1 file changed, 17 insertions(+), 51 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index b5f9c3511..5e6bd1a5e 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1101,23 +1101,6 @@ static u32 r8168ep_ocp_read(struct rtl8169_private *tp,
u8 mask, u16 reg)
return rtl_eri_read(tp, reg, ERIAR_OOB);
}
-static u32 ocp_read(struct rtl8169_private *tp, u8 mask, u16 reg)
-{
- switch (tp->mac_version) {
- case RTL_GIGA_MAC_VER_27:
- case RTL_GIGA_MAC_VER_28:
- case RTL_GIGA_MAC_VER_31:
- return r8168dp_ocp_read(tp, mask, reg);
- case RTL_GIGA_MAC_VER_49:
- case RTL_GIGA_MAC_VER_50:
- case RTL_GIGA_MAC_VER_51:
- return r8168ep_ocp_read(tp, mask, reg);
- default:
- BUG();
- return ~0;
- }
-}
-
static void r8168dp_ocp_write(struct rtl8169_private *tp, u8 mask, u16 reg,
u32 data)
{
@@ -1133,30 +1116,11 @@ static void r8168ep_ocp_write(struct rtl8169_private
*tp, u8 mask, u16 reg,
data, ERIAR_OOB);
}
-static void ocp_write(struct rtl8169_private *tp, u8 mask, u16 reg, u32 data)
-{
- switch (tp->mac_version) {
- case RTL_GIGA_MAC_VER_27:
- case RTL_GIGA_MAC_VER_28:
- case RTL_GIGA_MAC_VER_31:
- r8168dp_ocp_write(tp, mask, reg, data);
- break;
- case RTL_GIGA_MAC_VER_49:
- case RTL_GIGA_MAC_VER_50:
- case RTL_GIGA_MAC_VER_51:
- r8168ep_ocp_write(tp, mask, reg, data);
- break;
- default:
- BUG();
- break;
- }
-}
-
-static void rtl8168_oob_notify(struct rtl8169_private *tp, u8 cmd)
+static void r8168dp_oob_notify(struct rtl8169_private *tp, u8 cmd)
{
rtl_eri_write(tp, 0xe8, ERIAR_MASK_0001, cmd, ERIAR_EXGMAC);
- ocp_write(tp, 0x1, 0x30, 0x0001);
+ r8168dp_ocp_write(tp, 0x1, 0x30, 0x0001);
}
#define OOB_CMD_RESET 0x00
@@ -1168,18 +1132,18 @@ static u16 rtl8168_get_ocp_reg(struct rtl8169_private
*tp)
return (tp->mac_version == RTL_GIGA_MAC_VER_31) ? 0xb8 : 0x10;
}
-DECLARE_RTL_COND(rtl_ocp_read_cond)
+DECLARE_RTL_COND(rtl_dp_ocp_read_cond)
{
u16 reg;
reg = rtl8168_get_ocp_reg(tp);
- return ocp_read(tp, 0x0f, reg) & 0x0800;
+ return r8168dp_ocp_read(tp, 0x0f, reg) & 0x0800;
}
DECLARE_RTL_COND(rtl_ep_ocp_read_cond)
{
- return ocp_read(tp, 0x0f, 0x124) & 0x0001;
+ return r8168ep_ocp_read(tp, 0x0f, 0x124) & 0x0001;
}
DECLARE_RTL_COND(rtl_ocp_tx_cond)
@@ -1197,14 +1161,15 @@ static void rtl8168ep_stop_cmac(struct rtl8169_private
*tp)
static void rtl8168dp_driver_start(struct rtl8169_private *tp)
{
- rtl8168_oob_notify(tp, OOB_CMD_DRIVER_START);
- rtl_msleep_loop_wait_high(tp, _ocp_read_cond, 10, 10);
+ r8168dp_oob_notify(tp, OOB_CMD_DRIVER_START);
+ rtl_msleep_loop_wait_high(tp, _dp_ocp_read_cond, 10, 10);
}
static void rtl8168ep_driver_start(struct rtl8169_private *tp)
{
- ocp_write(tp, 0x01, 0x180, OOB_CMD_DRIVER_START);
- ocp_write(tp, 0x01, 0x30, ocp_read(tp, 0x01, 0x30) | 0x01);
+ r8168ep_ocp_write(tp, 0x01, 0x180, OOB_CMD_DRIVER_START);
+ r8168ep_ocp_write(tp, 0x01, 0x30,
+ r8168ep_ocp_read(tp, 0x01, 0x30) | 0x01);
rtl_msleep_loop_wait_high(tp, _ep_ocp_read_cond, 10, 10);
}
@@ -1229,15 +1194,16 @@ static void rtl8168_driver_start(struct rtl8169_private
*tp)
static void rtl8168dp_driver_stop(struct rtl8169_private *tp)
{
- rtl8168_oob_notify(tp, OOB_CMD_DRIVER_STOP);
- rtl_msleep_loop_wait_low(tp, _ocp_read_cond, 10, 10);
+ r8168dp_oob_notify(tp, OOB_CMD_DRIVER_STOP);
+ rtl_msleep_loop_wait_low(tp, _dp_ocp_read_cond, 10, 10);
}
static void rtl8168ep_driver_stop(struct rtl8169_private *tp)
{
rtl8168ep_stop_cmac(tp);
- ocp_write(tp, 0x01, 0x180, OOB_CMD_DRIVER_STOP);
- ocp_write(tp, 0x01, 0x30, ocp_read(tp, 0x01, 0x30) | 0x01);
+ r8168ep_ocp_write(tp, 0x01, 0x180, OOB_CMD_DRIVER_STOP);
+ r8168ep_ocp_write(tp, 0x01, 0x30,
+ r8168ep_ocp_read(tp, 0x01, 0x30) | 0x01);
rtl_msleep_loop_wait_low(tp, _ep_ocp_read_cond, 10, 10);
}
@@ -1264,12 +1230,12 @@ static bool r8168dp_check_dash(struct rtl8169_private
*tp)
{
u16 reg = rtl8168_get_ocp_reg(tp);
- return !!(ocp_read(tp, 0x0f, reg) & 0x8000);
+
[PATCH net-next 08/11] r8169: remove manual padding in struct ring_info
The compiler takes care of alignment and padding, I see no need to
bother him with manual hints.
Signed-off-by: Heiner Kallweit
---
drivers/net/ethernet/realtek/r8169.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index 3ffd9c18b..6736b804c 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -603,7 +603,6 @@ struct RxDesc {
struct ring_info {
struct sk_buff *skb;
u32 len;
- u8 __pad[sizeof(void *) - sizeof(u32)];
};
struct rtl8169_counters {
--
2.19.1
[PATCH net-next 02/11] r8169: use dev_get_drvdata where possible
Using dev_get_drvdata directly is simpler here.
Signed-off-by: Heiner Kallweit
---
drivers/net/ethernet/realtek/r8169.c | 15 +--
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index ff85f0282..fa6349b5d 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -6819,8 +6819,7 @@ static void rtl8169_net_suspend(struct net_device *dev)
static int rtl8169_suspend(struct device *device)
{
- struct pci_dev *pdev = to_pci_dev(device);
- struct net_device *dev = pci_get_drvdata(pdev);
+ struct net_device *dev = dev_get_drvdata(device);
struct rtl8169_private *tp = netdev_priv(dev);
rtl8169_net_suspend(dev);
@@ -6850,8 +6849,7 @@ static void __rtl8169_resume(struct net_device *dev)
static int rtl8169_resume(struct device *device)
{
- struct pci_dev *pdev = to_pci_dev(device);
- struct net_device *dev = pci_get_drvdata(pdev);
+ struct net_device *dev = dev_get_drvdata(device);
struct rtl8169_private *tp = netdev_priv(dev);
clk_prepare_enable(tp->clk);
@@ -6864,8 +6862,7 @@ static int rtl8169_resume(struct device *device)
static int rtl8169_runtime_suspend(struct device *device)
{
- struct pci_dev *pdev = to_pci_dev(device);
- struct net_device *dev = pci_get_drvdata(pdev);
+ struct net_device *dev = dev_get_drvdata(device);
struct rtl8169_private *tp = netdev_priv(dev);
if (!tp->TxDescArray)
@@ -6886,8 +6883,7 @@ static int rtl8169_runtime_suspend(struct device *device)
static int rtl8169_runtime_resume(struct device *device)
{
- struct pci_dev *pdev = to_pci_dev(device);
- struct net_device *dev = pci_get_drvdata(pdev);
+ struct net_device *dev = dev_get_drvdata(device);
struct rtl8169_private *tp = netdev_priv(dev);
rtl_rar_set(tp, dev->dev_addr);
@@ -6905,8 +6901,7 @@ static int rtl8169_runtime_resume(struct device *device)
static int rtl8169_runtime_idle(struct device *device)
{
- struct pci_dev *pdev = to_pci_dev(device);
- struct net_device *dev = pci_get_drvdata(pdev);
+ struct net_device *dev = dev_get_drvdata(device);
if (!netif_running(dev) || !netif_carrier_ok(dev))
pm_schedule_suspend(device, 1);
--
2.19.1
[PATCH net-next 01/11] r8169: merge rtl_irq_enable and rtl_irq_enable_all
After the recent changes to the interrupt handler rtl_irq_enable and
rtl_irq_enable_all can be merged.
Signed-off-by: Heiner Kallweit
---
drivers/net/ethernet/realtek/r8169.c | 13 -
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index 100731c86..ff85f0282 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1334,18 +1334,13 @@ static void rtl_irq_disable(struct rtl8169_private *tp)
mmiowb();
}
-static void rtl_irq_enable(struct rtl8169_private *tp, u16 bits)
-{
- RTL_W16(tp, IntrMask, bits);
-}
-
#define RTL_EVENT_NAPI_RX (RxOK | RxErr)
#define RTL_EVENT_NAPI_TX (TxOK | TxErr)
#define RTL_EVENT_NAPI (RTL_EVENT_NAPI_RX | RTL_EVENT_NAPI_TX)
-static void rtl_irq_enable_all(struct rtl8169_private *tp)
+static void rtl_irq_enable(struct rtl8169_private *tp)
{
- rtl_irq_enable(tp, RTL_EVENT_NAPI | tp->event_slow);
+ RTL_W16(tp, IntrMask, RTL_EVENT_NAPI | tp->event_slow);
}
static void rtl8169_irq_mask_and_ack(struct rtl8169_private *tp)
@@ -4643,7 +4638,7 @@ static void rtl_hw_start(struct rtl8169_private *tp)
rtl_set_rx_mode(tp->dev);
/* no early-rx interrupts */
RTL_W16(tp, MultiIntr, RTL_R16(tp, MultiIntr) & 0xf000);
- rtl_irq_enable_all(tp);
+ rtl_irq_enable(tp);
}
static void rtl_hw_start_8169(struct rtl8169_private *tp)
@@ -6533,7 +6528,7 @@ static int rtl8169_poll(struct napi_struct *napi, int
budget)
if (work_done < budget) {
napi_complete_done(napi, work_done);
- rtl_irq_enable_all(tp);
+ rtl_irq_enable(tp);
mmiowb();
}
--
2.19.1
[PATCH net-next 00/11] r8169: series with further smaller improvements
Again nothing exciting, just smaller improvements. Heiner Kallweit (11): r8169: merge rtl_irq_enable and rtl_irq_enable_all r8169: use dev_get_drvdata where possible r8169: remove unused interrupt sources r8169: replace event_slow with irq_mask r8169: use PCI_VDEVICE macro r8169: remove print_mac_version r8169: remove "not PCI Express" info message r8169: remove manual padding in struct ring_info r8169: remove workaround for ancient gcc bug r8169: simplify ocp functions r8169: improve chip version identification drivers/net/ethernet/realtek/r8169.c | 283 +++ 1 file changed, 112 insertions(+), 171 deletions(-) -- 2.19.1
[PATCH net-next] MAINTAINERS: Add myself as third phylib maintainer
Add myself as third phylib maintainer. Signed-off-by: Heiner Kallweit --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 7ff8865a9..2988ecbf6 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5540,6 +5540,7 @@ F:net/bridge/ ETHERNET PHY LIBRARY M: Andrew Lunn M: Florian Fainelli +M: Heiner Kallweit L: netdev@vger.kernel.org S: Maintained F: Documentation/ABI/testing/sysfs-bus-mdio -- 2.19.1
[PATCH net-next] net: phy: check for implementation of both callbacks in phy_drv_supports_irq
Now that the icplus driver has been fixed all PHY drivers supporting
interrupts have both callbacks (config_intr and ack_interrupt)
implemented - as it should be. Therefore phy_drv_supports_irq()
can be changed now to check for both callbacks being implemented.
Signed-off-by: Heiner Kallweit
---
drivers/net/phy/phy_device.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 55202a0ac..e06613f2d 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2122,7 +2122,7 @@ static void of_set_phy_eee_broken(struct phy_device
*phydev)
static bool phy_drv_supports_irq(struct phy_driver *phydrv)
{
- return phydrv->config_intr || phydrv->ack_interrupt;
+ return phydrv->config_intr && phydrv->ack_interrupt;
}
/**
--
2.19.1
[PATCH net-next] net: phy: switch to lockdep_assert_held in phylib
These checks are relevant during development / testing only,
therefore switch to lockdep_assert_held and friends.
Signed-off-by: Heiner Kallweit
---
drivers/net/phy/mdio_bus.c | 4 ++--
drivers/net/phy/phy.c| 2 +-
drivers/net/phy/phy_device.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 2e59a8419..5cb06f021 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -541,7 +541,7 @@ int __mdiobus_read(struct mii_bus *bus, int addr, u32
regnum)
{
int retval;
- WARN_ON_ONCE(!mutex_is_locked(>mdio_lock));
+ lockdep_assert_held_once(>mdio_lock);
retval = bus->read(bus, addr, regnum);
@@ -566,7 +566,7 @@ int __mdiobus_write(struct mii_bus *bus, int addr, u32
regnum, u16 val)
{
int err;
- WARN_ON_ONCE(!mutex_is_locked(>mdio_lock));
+ lockdep_assert_held_once(>mdio_lock);
err = bus->write(bus, addr, regnum, val);
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 083977d2f..b0b3c9ac4 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -514,7 +514,7 @@ static int phy_check_link_status(struct phy_device *phydev)
{
int err;
- WARN_ON(!mutex_is_locked(>lock));
+ lockdep_assert_held(>lock);
err = phy_read_status(phydev);
if (err)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 0f56d408b..1c20a0df3 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1340,7 +1340,7 @@ int __phy_resume(struct phy_device *phydev)
struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver);
int ret = 0;
- WARN_ON(!mutex_is_locked(>lock));
+ lockdep_assert_held(>lock);
if (phydev->drv && phydrv->resume)
ret = phydrv->resume(phydev);
--
2.19.1
[PATCH net-next] net: phy: icplus: add config_intr callback
Move IRQ configuration for IP101A/G from config_init to config_intr
callback. Reasons:
1. This allows phylib to disable interrupts if needed.
2. Icplus was the only driver supporting interrupts w/o defining a
config_intr callback. Now we can add a phylib plausibility check
disabling interrupt mode if one of the two irq-related callbacks
isn't defined.
I don't own hardware with this PHY, and the change is based on the
datasheet for IP101A LF (which is supposed to be register-compatible
with IP101A/G). Change is compile-tested only.
Signed-off-by: Heiner Kallweit
---
drivers/net/phy/icplus.c | 23 ---
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index 21ce68964..ad87bd328 100644
--- a/drivers/net/phy/icplus.c
+++ b/drivers/net/phy/icplus.c
@@ -42,8 +42,8 @@ MODULE_LICENSE("GPL");
#define IP1001_APS_ON 11 /* IP1001 APS Mode bit */
#define IP101A_G_APS_ON2 /* IP101A/G APS Mode
bit */
#define IP101A_G_IRQ_CONF_STATUS 0x11/* Conf Info IRQ & Status Reg */
-#defineIP101A_G_IRQ_PIN_USED (1<<15) /* INTR pin used */
-#defineIP101A_G_IRQ_DEFAULTIP101A_G_IRQ_PIN_USED
+#defineIP101A_G_IRQ_PIN_USED BIT(15) /* INTR pin used */
+#defineIP101A_G_NO_IRQ BIT(11) /* IRQ's inactive */
static int ip175c_config_init(struct phy_device *phydev)
{
@@ -170,11 +170,6 @@ static int ip101a_g_config_init(struct phy_device *phydev)
if (c < 0)
return c;
- /* INTR pin used: speed/link/duplex will cause an interrupt */
- c = phy_write(phydev, IP101A_G_IRQ_CONF_STATUS, IP101A_G_IRQ_DEFAULT);
- if (c < 0)
- return c;
-
/* Enable Auto Power Saving mode */
c = phy_read(phydev, IP10XX_SPEC_CTRL_STATUS);
c |= IP101A_G_APS_ON;
@@ -201,6 +196,19 @@ static int ip175c_config_aneg(struct phy_device *phydev)
return 0;
}
+static int ip101a_g_config_intr(struct phy_device *phydev)
+{
+ u16 val;
+
+ if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+ /* INTR pin used: Speed/link/duplex will cause an interrupt */
+ val = IP101A_G_IRQ_PIN_USED;
+ else
+ val = IP101A_G_NO_IRQ;
+
+ return phy_write(phydev, IP101A_G_IRQ_CONF_STATUS, val);
+}
+
static int ip101a_g_ack_interrupt(struct phy_device *phydev)
{
int err = phy_read(phydev, IP101A_G_IRQ_CONF_STATUS);
@@ -234,6 +242,7 @@ static struct phy_driver icplus_driver[] = {
.name = "ICPlus IP101A/G",
.phy_id_mask= 0x0ff0,
.features = PHY_BASIC_FEATURES,
+ .config_intr= ip101a_g_config_intr,
.ack_interrupt = ip101a_g_ack_interrupt,
.config_init= _g_config_init,
.suspend= genphy_suspend,
--
2.19.1
[PATCH net-next v3] PCI: add USR vendor id and use it in r8169 and w6692 driver
The PCI vendor id of U.S. Robotics isn't defined in pci_ids.h so far,
only ISDN driver w6692 has a private definition. Move the definition
to pci_ids.h and use it in the r8169 driver too.
Signed-off-by: Heiner Kallweit
---
v2:
- The original patch caused a build failure in w6692 driver because
it broke the private PCI device id definition.
v3:
- Don't move device id's to pci_ids.h because they are used in
one module only respectively.
---
drivers/isdn/hardware/mISDN/w6692.c | 3 ---
drivers/net/ethernet/realtek/r8169.c | 2 +-
include/linux/pci_ids.h | 2 ++
3 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/isdn/hardware/mISDN/w6692.c
b/drivers/isdn/hardware/mISDN/w6692.c
index 5acf6ab67..6f60aced1 100644
--- a/drivers/isdn/hardware/mISDN/w6692.c
+++ b/drivers/isdn/hardware/mISDN/w6692.c
@@ -52,10 +52,7 @@ static const struct w6692map w6692_map[] =
{W6692_USR, "USR W6692"}
};
-#ifndef PCI_VENDOR_ID_USR
-#define PCI_VENDOR_ID_USR 0x16ec
#define PCI_DEVICE_ID_USR_6692 0x3409
-#endif
struct w6692_ch {
struct bchannel bch;
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index 1fd01688d..366a690eb 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -224,7 +224,7 @@ static const struct pci_device_id rtl8169_pci_tbl[] = {
{ PCI_DEVICE(PCI_VENDOR_ID_DLINK, 0x4300), 0, 0, RTL_CFG_0 },
{ PCI_DEVICE(PCI_VENDOR_ID_DLINK, 0x4302), 0, 0, RTL_CFG_0 },
{ PCI_DEVICE(PCI_VENDOR_ID_AT, 0xc107), 0, 0, RTL_CFG_0 },
- { PCI_DEVICE(0x16ec,0x0116), 0, 0, RTL_CFG_0 },
+ { PCI_DEVICE(PCI_VENDOR_ID_USR, 0x0116), 0, 0, RTL_CFG_0 },
{ PCI_VENDOR_ID_LINKSYS,0x1032,
PCI_ANY_ID, 0x0024, 0, 0, RTL_CFG_0 },
{ 0x0001, 0x8168,
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 69f0abe1b..144de2e89 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2359,6 +2359,8 @@
#define PCI_VENDOR_ID_SYNOPSYS 0x16c3
+#define PCI_VENDOR_ID_USR 0x16ec
+
#define PCI_VENDOR_ID_VITESSE 0x1725
#define PCI_DEVICE_ID_VITESSE_VSC7174 0x7174
--
2.19.1
[PATCH net-next v2] PCI: add USR vendor id and use it in r8169 and w6692 driver
The PCI vendor id of U.S. Robotics isn't defined in pci_ids.h so far,
only ISDN driver w6692 has a private definition. Move the definition
to pci_ids.h and use it also in the r8169 driver.
Signed-off-by: Heiner Kallweit
---
v2:
- The original patch caused a build failure in w6692 driver because
it broke the private PCI device id definition.
I hope it's ok to submit this through the netdev tree.
---
drivers/isdn/hardware/mISDN/w6692.c | 5 -
drivers/net/ethernet/realtek/r8169.c | 2 +-
include/linux/pci_ids.h | 4
3 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/isdn/hardware/mISDN/w6692.c
b/drivers/isdn/hardware/mISDN/w6692.c
index 5acf6ab67..f6b4f4f11 100644
--- a/drivers/isdn/hardware/mISDN/w6692.c
+++ b/drivers/isdn/hardware/mISDN/w6692.c
@@ -52,11 +52,6 @@ static const struct w6692map w6692_map[] =
{W6692_USR, "USR W6692"}
};
-#ifndef PCI_VENDOR_ID_USR
-#define PCI_VENDOR_ID_USR 0x16ec
-#define PCI_DEVICE_ID_USR_6692 0x3409
-#endif
-
struct w6692_ch {
struct bchannel bch;
u32 addr;
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index 1fd01688d..366a690eb 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -224,7 +224,7 @@ static const struct pci_device_id rtl8169_pci_tbl[] = {
{ PCI_DEVICE(PCI_VENDOR_ID_DLINK, 0x4300), 0, 0, RTL_CFG_0 },
{ PCI_DEVICE(PCI_VENDOR_ID_DLINK, 0x4302), 0, 0, RTL_CFG_0 },
{ PCI_DEVICE(PCI_VENDOR_ID_AT, 0xc107), 0, 0, RTL_CFG_0 },
- { PCI_DEVICE(0x16ec,0x0116), 0, 0, RTL_CFG_0 },
+ { PCI_DEVICE(PCI_VENDOR_ID_USR, 0x0116), 0, 0, RTL_CFG_0 },
{ PCI_VENDOR_ID_LINKSYS,0x1032,
PCI_ANY_ID, 0x0024, 0, 0, RTL_CFG_0 },
{ 0x0001, 0x8168,
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 69f0abe1b..1fac231fe 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2359,6 +2359,10 @@
#define PCI_VENDOR_ID_SYNOPSYS 0x16c3
+#define PCI_VENDOR_ID_USR 0x16ec
+#define PCI_DEVICE_ID_USR_997902 0x0116
+#define PCI_DEVICE_ID_USR_6692 0x3409
+
#define PCI_VENDOR_ID_VITESSE 0x1725
#define PCI_DEVICE_ID_VITESSE_VSC7174 0x7174
--
2.19.1
[PATCH net-next 2/2] r8169: use proper constant for PCI vendor USR instead of numerical id
Use proper constant for PCI vendor USR instead of numerical id.
Signed-off-by: Heiner Kallweit
---
drivers/net/ethernet/realtek/r8169.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index 1fd01688d..366a690eb 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -224,7 +224,7 @@ static const struct pci_device_id rtl8169_pci_tbl[] = {
{ PCI_DEVICE(PCI_VENDOR_ID_DLINK, 0x4300), 0, 0, RTL_CFG_0 },
{ PCI_DEVICE(PCI_VENDOR_ID_DLINK, 0x4302), 0, 0, RTL_CFG_0 },
{ PCI_DEVICE(PCI_VENDOR_ID_AT, 0xc107), 0, 0, RTL_CFG_0 },
- { PCI_DEVICE(0x16ec,0x0116), 0, 0, RTL_CFG_0 },
+ { PCI_DEVICE(PCI_VENDOR_ID_USR, 0x0116), 0, 0, RTL_CFG_0 },
{ PCI_VENDOR_ID_LINKSYS,0x1032,
PCI_ANY_ID, 0x0024, 0, 0, RTL_CFG_0 },
{ 0x0001, 0x8168,
--
2.19.1
[PATCH net-next 1/2] PCI: add USR vendor id
Add U.S. Robotics (USR) PCI vendor id. Signed-off-by: Heiner Kallweit --- include/linux/pci_ids.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h index 69f0abe1b..144de2e89 100644 --- a/include/linux/pci_ids.h +++ b/include/linux/pci_ids.h @@ -2359,6 +2359,8 @@ #define PCI_VENDOR_ID_SYNOPSYS 0x16c3 +#define PCI_VENDOR_ID_USR 0x16ec + #define PCI_VENDOR_ID_VITESSE 0x1725 #define PCI_DEVICE_ID_VITESSE_VSC7174 0x7174 -- 2.19.1
[PATCH net-next 0/2] r8169: add USR PCI vendor id
Add constant for U.S. Robotics (USR) PCI id and use it in the r8169 driver. Alternatively this change could go thorugh the PCI tree, up to Bjorn/David. Heiner Kallweit (2): PCI: add USR vendor id r8169: use proper constant for PCI vendor USR instead of id drivers/net/ethernet/realtek/r8169.c | 2 +- include/linux/pci_ids.h | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) -- 2.19.1
[PATCH net-next] net: phy: remove states PHY_STARTING and PHY_PENDING
Both states aren't used. Most likely they result from an idea that
never materialized. So remove them.
Signed-off-by: Heiner Kallweit
---
drivers/net/phy/phy.c | 7 ---
include/linux/phy.h | 22 ++
2 files changed, 2 insertions(+), 27 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 62a7105c3..4f2606d50 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -46,9 +46,7 @@ static const char *phy_state_to_str(enum phy_state st)
{
switch (st) {
PHY_STATE_STR(DOWN)
- PHY_STATE_STR(STARTING)
PHY_STATE_STR(READY)
- PHY_STATE_STR(PENDING)
PHY_STATE_STR(UP)
PHY_STATE_STR(RUNNING)
PHY_STATE_STR(NOLINK)
@@ -852,9 +850,6 @@ void phy_start(struct phy_device *phydev)
mutex_lock(>lock);
switch (phydev->state) {
- case PHY_STARTING:
- phydev->state = PHY_PENDING;
- break;
case PHY_READY:
phydev->state = PHY_UP;
break;
@@ -902,9 +897,7 @@ void phy_state_machine(struct work_struct *work)
switch (phydev->state) {
case PHY_DOWN:
- case PHY_STARTING:
case PHY_READY:
- case PHY_PENDING:
break;
case PHY_UP:
needs_aneg = true;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 0595f1e99..aa8f797b4 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -271,29 +271,13 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int
addr);
* DOWN: PHY device and driver are not ready for anything. probe
* should be called if and only if the PHY is in this state,
* given that the PHY device exists.
- * - PHY driver probe function will, depending on the PHY, set
- * the state to STARTING or READY
- *
- * STARTING: PHY device is coming up, and the ethernet driver is
- * not ready. PHY drivers may set this in the probe function.
- * If they do, they are responsible for making sure the state is
- * eventually set to indicate whether the PHY is UP or READY,
- * depending on the state when the PHY is done starting up.
- * - PHY driver will set the state to READY
- * - start will set the state to PENDING
+ * - PHY driver probe function will set the state to READY
*
* READY: PHY is ready to send and receive packets, but the
* controller is not. By default, PHYs which do not implement
- * probe will be set to this state by phy_probe(). If the PHY
- * driver knows the PHY is ready, and the PHY state is STARTING,
- * then it sets this STATE.
+ * probe will be set to this state by phy_probe().
* - start will set the state to UP
*
- * PENDING: PHY device is coming up, but the ethernet driver is
- * ready. phy_start will set this state if the PHY state is
- * STARTING.
- * - PHY driver will set the state to UP when the PHY is ready
- *
* UP: The PHY and attached device are ready to do work.
* Interrupts should be started here.
* - timer moves to NOLINK or RUNNING
@@ -330,9 +314,7 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int
addr);
*/
enum phy_state {
PHY_DOWN = 0,
- PHY_STARTING,
PHY_READY,
- PHY_PENDING,
PHY_UP,
PHY_RUNNING,
PHY_NOLINK,
--
2.19.1
[PATCH net-next 2/2] net: phy: realtek: use new PHYID matching macros
Use new macros for PHYID matching to avoid boilerplate code.
Signed-off-by: Heiner Kallweit
---
drivers/net/phy/realtek.c | 29 ++---
1 file changed, 10 insertions(+), 19 deletions(-)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 0f8e5b1c9..c6010fb1a 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -213,14 +213,12 @@ static int rtl8366rb_config_init(struct phy_device
*phydev)
static struct phy_driver realtek_drvs[] = {
{
- .phy_id = 0x8201,
+ PHY_ID_MATCH_EXACT(0x8201),
.name = "RTL8201CP Ethernet",
- .phy_id_mask= 0x,
.features = PHY_BASIC_FEATURES,
}, {
- .phy_id = 0x001cc816,
+ PHY_ID_MATCH_EXACT(0x001cc816),
.name = "RTL8201F Fast Ethernet",
- .phy_id_mask= 0x001f,
.features = PHY_BASIC_FEATURES,
.ack_interrupt = _ack_interrupt,
.config_intr= _config_intr,
@@ -229,17 +227,15 @@ static struct phy_driver realtek_drvs[] = {
.read_page = rtl821x_read_page,
.write_page = rtl821x_write_page,
}, {
- .phy_id = 0x001cc910,
+ PHY_ID_MATCH_EXACT(0x001cc910),
.name = "RTL8211 Gigabit Ethernet",
- .phy_id_mask= 0x001f,
.features = PHY_GBIT_FEATURES,
.config_aneg= rtl8211_config_aneg,
.read_mmd = _read_mmd_unsupported,
.write_mmd = _write_mmd_unsupported,
}, {
- .phy_id = 0x001cc912,
+ PHY_ID_MATCH_EXACT(0x001cc912),
.name = "RTL8211B Gigabit Ethernet",
- .phy_id_mask= 0x001f,
.features = PHY_GBIT_FEATURES,
.ack_interrupt = _ack_interrupt,
.config_intr= _config_intr,
@@ -248,35 +244,31 @@ static struct phy_driver realtek_drvs[] = {
.suspend= rtl8211b_suspend,
.resume = rtl8211b_resume,
}, {
- .phy_id = 0x001cc913,
+ PHY_ID_MATCH_EXACT(0x001cc913),
.name = "RTL8211C Gigabit Ethernet",
- .phy_id_mask= 0x001f,
.features = PHY_GBIT_FEATURES,
.config_init= rtl8211c_config_init,
.read_mmd = _read_mmd_unsupported,
.write_mmd = _write_mmd_unsupported,
}, {
- .phy_id = 0x001cc914,
+ PHY_ID_MATCH_EXACT(0x001cc914),
.name = "RTL8211DN Gigabit Ethernet",
- .phy_id_mask= 0x001f,
.features = PHY_GBIT_FEATURES,
.ack_interrupt = rtl821x_ack_interrupt,
.config_intr= rtl8211e_config_intr,
.suspend= genphy_suspend,
.resume = genphy_resume,
}, {
- .phy_id = 0x001cc915,
+ PHY_ID_MATCH_EXACT(0x001cc915),
.name = "RTL8211E Gigabit Ethernet",
- .phy_id_mask= 0x001f,
.features = PHY_GBIT_FEATURES,
.ack_interrupt = _ack_interrupt,
.config_intr= _config_intr,
.suspend= genphy_suspend,
.resume = genphy_resume,
}, {
- .phy_id = 0x001cc916,
+ PHY_ID_MATCH_EXACT(0x001cc916),
.name = "RTL8211F Gigabit Ethernet",
- .phy_id_mask= 0x001f,
.features = PHY_GBIT_FEATURES,
.config_init= _config_init,
.ack_interrupt = _ack_interrupt,
@@ -286,9 +278,8 @@ static struct phy_driver realtek_drvs[] = {
.read_page = rtl821x_read_page,
.write_page = rtl821x_write_page,
}, {
- .phy_id = 0x001cc961,
+ PHY_ID_MATCH_EXACT(0x001cc961),
.name = "RTL8366RB Gigabit Ethernet",
- .phy_id_mask= 0x001f,
.features = PHY_GBIT_FEATURES,
.config_init= _config_init,
.suspend= genphy_suspend,
@@ -299,7 +290,7 @@ static struct phy_driver realtek_drvs[] = {
module_phy_driver(realtek_drvs);
static const struct mdio_device_id __maybe_unused realtek_tbl[] = {
- { 0x001cc800, GENMASK(31, 10) },
+ { PHY_ID_MATCH_VENDOR(0x001cc800) },
{ }
};
--
2.19.1
[PATCH net-next 1/2] net: phy: add macros for PHYID matching
Add macros for PHYID matching to be used in PHY driver configs.
By using these macros some boilerplate code can be avoided.
Signed-off-by: Heiner Kallweit
---
include/linux/phy.h | 4
1 file changed, 4 insertions(+)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 17d1f6472..03005c65e 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -651,6 +651,10 @@ struct phy_driver {
#define PHY_ANY_ID "MATCH ANY PHY"
#define PHY_ANY_UID 0x
+#define PHY_ID_MATCH_EXACT(id) .phy_id = (id), .phy_id_mask = GENMASK(31, 0)
+#define PHY_ID_MATCH_MODEL(id) .phy_id = (id), .phy_id_mask = GENMASK(31, 4)
+#define PHY_ID_MATCH_VENDOR(id) .phy_id = (id), .phy_id_mask = GENMASK(31, 10)
+
/* A Structure for boards to register fixups with the PHY Lib */
struct phy_fixup {
struct list_head list;
--
2.19.1
[PATCH net-next 0/2] net: phy: add macros for PHYID matching in PHY driver config
Add macros for PHYID matching to be used in PHY driver configs. By using these macros some boilerplate code can be avoided. Use them initially in the Realtek PHY drivers. Heiner Kallweit (2): net: phy: add macros for PHYID matching net: phy: realtek: use new PHYID matching macros drivers/net/phy/realtek.c | 29 ++--- include/linux/phy.h | 4 2 files changed, 14 insertions(+), 19 deletions(-) -- 2.19.1
[PATCH net-next 3/3] net: phy: improve and inline phy_change
Now that phy_mac_interrupt() doesn't call phy_change() any longer it's
called from phy_interrupt() only. Therefore phy_interrupt_is_valid()
returns true always and the check can be removed.
In case of PHY_HALTED phy_interrupt() bails out immediately,
therefore the second check for PHY_HALTED including the call to
phy_disable_interrupts() can be removed.
Signed-off-by: Heiner Kallweit
---
drivers/net/phy/phy.c | 47 ++-
1 file changed, 15 insertions(+), 32 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index ce1e8130a..083977d2f 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -722,41 +722,12 @@ static int phy_disable_interrupts(struct phy_device
*phydev)
return phy_clear_interrupt(phydev);
}
-/**
- * phy_change - Called by the phy_interrupt to handle PHY changes
- * @phydev: phy_device struct that interrupted
- */
-static irqreturn_t phy_change(struct phy_device *phydev)
-{
- if (phy_interrupt_is_valid(phydev)) {
- if (phydev->drv->did_interrupt &&
- !phydev->drv->did_interrupt(phydev))
- return IRQ_NONE;
-
- if (phydev->state == PHY_HALTED)
- if (phy_disable_interrupts(phydev))
- goto phy_err;
- }
-
- /* reschedule state queue work to run as soon as possible */
- phy_trigger_machine(phydev);
-
- if (phy_interrupt_is_valid(phydev) && phy_clear_interrupt(phydev))
- goto phy_err;
- return IRQ_HANDLED;
-
-phy_err:
- phy_error(phydev);
- return IRQ_NONE;
-}
-
/**
* phy_interrupt - PHY interrupt handler
* @irq: interrupt line
* @phy_dat: phy_device pointer
*
- * Description: When a PHY interrupt occurs, the handler disables
- * interrupts, and uses phy_change to handle the interrupt.
+ * Description: Handle PHY interrupt
*/
static irqreturn_t phy_interrupt(int irq, void *phy_dat)
{
@@ -765,7 +736,19 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
if (PHY_HALTED == phydev->state)
return IRQ_NONE;/* It can't be ours. */
- return phy_change(phydev);
+ if (phydev->drv->did_interrupt && !phydev->drv->did_interrupt(phydev))
+ return IRQ_NONE;
+
+ /* reschedule state queue work to run as soon as possible */
+ phy_trigger_machine(phydev);
+
+ if (phy_clear_interrupt(phydev))
+ goto phy_err;
+ return IRQ_HANDLED;
+
+phy_err:
+ phy_error(phydev);
+ return IRQ_NONE;
}
/**
@@ -846,7 +829,7 @@ void phy_stop(struct phy_device *phydev)
phy_state_machine(>state_queue.work);
/* Cannot call flush_scheduled_work() here as desired because
-* of rtnl_lock(), but PHY_HALTED shall guarantee phy_change()
+* of rtnl_lock(), but PHY_HALTED shall guarantee irq handler
* will not reenable interrupts.
*/
}
--
2.19.1
[PATCH net-next 2/3] net: phy: simplify phy_mac_interrupt and related functions
When using phy_mac_interrupt() the irq number is set to
PHY_IGNORE_INTERRUPT, therefore phy_interrupt_is_valid() returns false.
As a result phy_change() effectively just calls phy_trigger_machine()
when called from phy_mac_interrupt() via phy_change_work(). So we can
call phy_trigger_machine() from phy_mac_interrupt() directly and
remove some now unneeded code.
Signed-off-by: Heiner Kallweit
---
drivers/net/phy/phy.c| 14 +-
drivers/net/phy/phy_device.c | 1 -
include/linux/phy.h | 3 ---
3 files changed, 1 insertion(+), 17 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index da41420df..ce1e8130a 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -750,18 +750,6 @@ static irqreturn_t phy_change(struct phy_device *phydev)
return IRQ_NONE;
}
-/**
- * phy_change_work - Scheduled by the phy_mac_interrupt to handle PHY changes
- * @work: work_struct that describes the work to be done
- */
-void phy_change_work(struct work_struct *work)
-{
- struct phy_device *phydev =
- container_of(work, struct phy_device, phy_queue);
-
- phy_change(phydev);
-}
-
/**
* phy_interrupt - PHY interrupt handler
* @irq: interrupt line
@@ -1005,7 +993,7 @@ void phy_state_machine(struct work_struct *work)
void phy_mac_interrupt(struct phy_device *phydev)
{
/* Trigger a state machine change */
- queue_work(system_power_efficient_wq, >phy_queue);
+ phy_trigger_machine(phydev);
}
EXPORT_SYMBOL(phy_mac_interrupt);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 00a46218c..0f56d408b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -587,7 +587,6 @@ struct phy_device *phy_device_create(struct mii_bus *bus,
int addr, int phy_id,
mutex_init(>lock);
INIT_DELAYED_WORK(>state_queue, phy_state_machine);
- INIT_WORK(>phy_queue, phy_change_work);
/* Request the appropriate module unconditionally; don't
* bother trying to do so only if it isn't already loaded,
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 7db07e69c..17d1f6472 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -369,7 +369,6 @@ struct phy_c45_device_ids {
* giving up on the current attempt at acquiring a link
* irq: IRQ number of the PHY's interrupt (-1 if none)
* phy_timer: The timer for handling the state machine
- * phy_queue: A work_queue for the phy_mac_interrupt
* attached_dev: The attached enet driver's device instance ptr
* adjust_link: Callback for the enet controller to respond to
* changes in the link state.
@@ -454,7 +453,6 @@ struct phy_device {
void *priv;
/* Interrupt and Polling infrastructure */
- struct work_struct phy_queue;
struct delayed_work state_queue;
struct mutex lock;
@@ -1029,7 +1027,6 @@ int phy_driver_register(struct phy_driver *new_driver,
struct module *owner);
int phy_drivers_register(struct phy_driver *new_driver, int n,
struct module *owner);
void phy_state_machine(struct work_struct *work);
-void phy_change_work(struct work_struct *work);
void phy_mac_interrupt(struct phy_device *phydev);
void phy_start_machine(struct phy_device *phydev);
void phy_stop_machine(struct phy_device *phydev);
--
2.19.1
[PATCH net-next 1/3] net: phy: don't set state PHY_CHANGELINK in phy_change
State PHY_CHANGELINK isn't needed here, we can call the state machine directly. We just have to remove the check for phy_polling_mode() to make this work also in interrupt mode. Removing this check doesn't cause any overhead because when not polling the state machine is called only if required by some event. Signed-off-by: Heiner Kallweit --- drivers/net/phy/phy.c | 8 include/linux/phy.h | 7 ++- 2 files changed, 2 insertions(+), 13 deletions(-) diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 8dac890f3..da41420df 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -738,11 +738,6 @@ static irqreturn_t phy_change(struct phy_device *phydev) goto phy_err; } - mutex_lock(>lock); - if ((PHY_RUNNING == phydev->state) || (PHY_NOLINK == phydev->state)) - phydev->state = PHY_CHANGELINK; - mutex_unlock(>lock); - /* reschedule state queue work to run as soon as possible */ phy_trigger_machine(phydev); @@ -946,9 +941,6 @@ void phy_state_machine(struct work_struct *work) break; case PHY_NOLINK: case PHY_RUNNING: - if (!phy_polling_mode(phydev)) - break; - /* fall through */ case PHY_CHANGELINK: case PHY_RESUMING: err = phy_check_link_status(phydev); diff --git a/include/linux/phy.h b/include/linux/phy.h index 59bb31ee1..7db07e69c 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -298,7 +298,7 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr); * - timer moves to NOLINK or RUNNING * * NOLINK: PHY is up, but not currently plugged in. - * - If the timer notes that the link comes back, we move to RUNNING + * - irq or timer will set RUNNING if link comes back * - phy_stop moves to HALTED * * FORCING: PHY is being configured with forced settings @@ -309,10 +309,7 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr); * * RUNNING: PHY is currently up, running, and possibly sending * and/or receiving packets - * - timer will set CHANGELINK if we're polling (this ensures the - * link state is polled every other cycle of this state machine, - * which makes it every other second) - * - irq will set CHANGELINK + * - irq or timer will set NOLINK if link goes down * - phy_stop moves to HALTED * * CHANGELINK: PHY experienced a change in link state -- 2.19.1
[PATCH net-next 0/3] net: phy: further phylib simplifications after recent changes to the state machine
After the recent changes to the state machine phylib can be further simplified (w/o having to make any assumptions). Heiner Kallweit (3): net: phy: don't set state PHY_CHANGELINK in phy_change net: phy: simplify phy_mac_interrupt and related functions net: phy: improve and inline phy_change drivers/net/phy/phy.c| 67 drivers/net/phy/phy_device.c | 1 - include/linux/phy.h | 10 ++ 3 files changed, 17 insertions(+), 61 deletions(-) -- 2.19.1
[PATCH v2 net-next] net: phy: improve struct phy_device member interrupts handling
As a heritage from the very early days of phylib member interrupts is
defined as u32 even though it's just a flag whether interrupts are
enabled. So we can change it to a bitfield member. In addition change
the code dealing with this member in a way that it's clear we're
dealing with a bool value.
Signed-off-by: Heiner Kallweit
---
v2:
- use false/true instead of 0/1 for the constants
Actually this member isn't needed at all and could be replaced with
a parameter in phy_driver->config_intr. But this would mean an API
change, maybe I come up with a proposal later.
---
drivers/net/phy/phy.c | 4 ++--
include/linux/phy.h | 10 +-
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index dd5bff955..8dac890f3 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -115,9 +115,9 @@ static int phy_clear_interrupt(struct phy_device *phydev)
*
* Returns 0 on success or < 0 on error.
*/
-static int phy_config_interrupt(struct phy_device *phydev, u32 interrupts)
+static int phy_config_interrupt(struct phy_device *phydev, bool interrupts)
{
- phydev->interrupts = interrupts;
+ phydev->interrupts = interrupts ? 1 : 0;
if (phydev->drv->config_intr)
return phydev->drv->config_intr(phydev);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 240e04d5a..59bb31ee1 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -262,8 +262,8 @@ static inline struct mii_bus *devm_mdiobus_alloc(struct
device *dev)
void devm_mdiobus_free(struct device *dev, struct mii_bus *bus);
struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
-#define PHY_INTERRUPT_DISABLED 0x0
-#define PHY_INTERRUPT_ENABLED 0x8000
+#define PHY_INTERRUPT_DISABLED false
+#define PHY_INTERRUPT_ENABLED true
/* PHY state machine states:
*
@@ -409,6 +409,9 @@ struct phy_device {
/* The most recently read link state */
unsigned link:1;
+ /* Interrupts are enabled */
+ unsigned interrupts:1;
+
enum phy_state state;
u32 dev_flags;
@@ -424,9 +427,6 @@ struct phy_device {
int pause;
int asym_pause;
- /* Enabled Interrupts */
- u32 interrupts;
-
/* Union of PHY and Attached devices' supported modes */
/* See mii.h for more info */
u32 supported;
--
2.19.1
Re: [PATCH net-next 1/2] net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt
On 08.11.2018 23:33, Florian Fainelli wrote: > On 11/8/18 1:55 PM, Heiner Kallweit wrote: >> Flag PHY_HAS_INTERRUPT is used only here for this small check. I think >> using interrupts isn't possible if a driver defines neither >> config_intr nor ack_interrupts callback. So we can replace checking >> flag PHY_HAS_INTERRUPT with checking for these callbacks. >> >> Signed-off-by: Heiner Kallweit >> --- >> drivers/net/phy/phy_device.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c >> index d165a2c82..33e51b955 100644 >> --- a/drivers/net/phy/phy_device.c >> +++ b/drivers/net/phy/phy_device.c >> @@ -2104,8 +2104,9 @@ static int phy_probe(struct device *dev) >> /* Disable the interrupt if the PHY doesn't support it >> * but the interrupt is still a valid one >> */ >> -if (!(phydrv->flags & PHY_HAS_INTERRUPT) && >> -phy_interrupt_is_valid(phydev)) >> + if (!phydrv->config_intr && >> + !phydrv->ack_interrupt && >> + phy_interrupt_is_valid(phydev)) >> phydev->irq = PHY_POLL; > > I would introduce an inline helper function which checks for > drv->config_intr and config_ack_interrupt, that way if we ever have to > introduce an additional function in the future, we just update the > helper to check for that. > > Other than that, LGTM > OK, will add a helper and remove PHY_HAS_INTERRUPT from all drivers.
Re: [PATCH net-next] net: phy: improve struct phy_device member interrupts handling
On 08.11.2018 23:24, Andrew Lunn wrote:
> On Thu, Nov 08, 2018 at 10:36:33PM +0100, Heiner Kallweit wrote:
>> As a heritage from the very early days of phylib member interrupts is
>> defined as u32 even though it's just a flag whether interrupts are
>> enabled. So we can change it to a bitfield member. In addition change
>> the code dealing with this member in a way that it's clear we're
>> dealing with a bool value.
>>
>> Signed-off-by: Heiner Kallweit
>> ---
>> Actually this member isn't needed at all and could be replaced with
>> a parameter in phy_driver->config_intr. But this would mean an API
>> change, maybe I come up with a proposal later.
>> ---
>> drivers/net/phy/phy.c | 2 +-
>> include/linux/phy.h | 10 +-
>> 2 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index dd5bff955..a5e6acfe6 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -115,7 +115,7 @@ static int phy_clear_interrupt(struct phy_device *phydev)
>> *
>> * Returns 0 on success or < 0 on error.
>> */
>> -static int phy_config_interrupt(struct phy_device *phydev, u32 interrupts)
>> +static int phy_config_interrupt(struct phy_device *phydev, bool interrupts)
>> {
>> phydev->interrupts = interrupts;
>> if (phydev->drv->config_intr)
>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>> index 5dd85c441..fc90af152 100644
>> --- a/include/linux/phy.h
>> +++ b/include/linux/phy.h
>> @@ -263,8 +263,8 @@ static inline struct mii_bus *devm_mdiobus_alloc(struct
>> device *dev)
>> void devm_mdiobus_free(struct device *dev, struct mii_bus *bus);
>> struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
>>
>> -#define PHY_INTERRUPT_DISABLED 0x0
>> -#define PHY_INTERRUPT_ENABLED 0x8000
>> +#define PHY_INTERRUPT_DISABLED 0
>> +#define PHY_INTERRUPT_ENABLED 1
>
> Hi Heiner
>
> Since this is passed around as a bool, false/true would be better than
> 0/1.
>
I thought about that before submitting the patch. I preferred to go with
0/1 because we assign this value to a 1 bit bitfield member which supports
values 0/1 only. So neither option is perfect IMO.
> Andrew
>
Heiner
[PATCH net-next 2/2] net: phy: realtek: remove flag PHY_HAS_INTERRUPT from driver configs
Now that flag PHY_HAS_INTERRUPT has been replaced with a check for
callbacks config_intr and ack_interrupt, we can remove setting this
flag from driver configs.
Signed-off-by: Heiner Kallweit
---
drivers/net/phy/realtek.c | 7 ---
1 file changed, 7 deletions(-)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index abff4cdc9..3985b4a4d 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -216,12 +216,10 @@ static struct phy_driver realtek_drvs[] = {
.phy_id = 0x8201,
.name = "RTL8201CP Ethernet",
.features = PHY_BASIC_FEATURES,
- .flags = PHY_HAS_INTERRUPT,
}, {
.phy_id = 0x001cc816,
.name = "RTL8201F Fast Ethernet",
.features = PHY_BASIC_FEATURES,
- .flags = PHY_HAS_INTERRUPT,
.ack_interrupt = _ack_interrupt,
.config_intr= _config_intr,
.suspend= genphy_suspend,
@@ -239,7 +237,6 @@ static struct phy_driver realtek_drvs[] = {
.phy_id = 0x001cc912,
.name = "RTL8211B Gigabit Ethernet",
.features = PHY_GBIT_FEATURES,
- .flags = PHY_HAS_INTERRUPT,
.ack_interrupt = _ack_interrupt,
.config_intr= _config_intr,
.read_mmd = _read_mmd_unsupported,
@@ -257,7 +254,6 @@ static struct phy_driver realtek_drvs[] = {
.phy_id = 0x001cc914,
.name = "RTL8211DN Gigabit Ethernet",
.features = PHY_GBIT_FEATURES,
- .flags = PHY_HAS_INTERRUPT,
.ack_interrupt = rtl821x_ack_interrupt,
.config_intr= rtl8211e_config_intr,
.suspend= genphy_suspend,
@@ -266,7 +262,6 @@ static struct phy_driver realtek_drvs[] = {
.phy_id = 0x001cc915,
.name = "RTL8211E Gigabit Ethernet",
.features = PHY_GBIT_FEATURES,
- .flags = PHY_HAS_INTERRUPT,
.ack_interrupt = _ack_interrupt,
.config_intr= _config_intr,
.suspend= genphy_suspend,
@@ -275,7 +270,6 @@ static struct phy_driver realtek_drvs[] = {
.phy_id = 0x001cc916,
.name = "RTL8211F Gigabit Ethernet",
.features = PHY_GBIT_FEATURES,
- .flags = PHY_HAS_INTERRUPT,
.config_init= _config_init,
.ack_interrupt = _ack_interrupt,
.config_intr= _config_intr,
@@ -287,7 +281,6 @@ static struct phy_driver realtek_drvs[] = {
.phy_id = 0x001cc961,
.name = "RTL8366RB Gigabit Ethernet",
.features = PHY_GBIT_FEATURES,
- .flags = PHY_HAS_INTERRUPT,
.config_init= _config_init,
.suspend= genphy_suspend,
.resume = genphy_resume,
--
2.19.1
[PATCH net-next 1/2] net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt
Flag PHY_HAS_INTERRUPT is used only here for this small check. I think using interrupts isn't possible if a driver defines neither config_intr nor ack_interrupts callback. So we can replace checking flag PHY_HAS_INTERRUPT with checking for these callbacks. Signed-off-by: Heiner Kallweit --- drivers/net/phy/phy_device.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index d165a2c82..33e51b955 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -2104,8 +2104,9 @@ static int phy_probe(struct device *dev) /* Disable the interrupt if the PHY doesn't support it * but the interrupt is still a valid one */ - if (!(phydrv->flags & PHY_HAS_INTERRUPT) && - phy_interrupt_is_valid(phydev)) +if (!phydrv->config_intr && +!phydrv->ack_interrupt && +phy_interrupt_is_valid(phydev)) phydev->irq = PHY_POLL; if (phydrv->flags & PHY_IS_INTERNAL) -- 2.19.1
[PATCH net-next 0/2] net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt
Flag PHY_HAS_INTERRUPT is used only here for this small check. I think using interrupts isn't possible if a driver defines neither config_intr nor ack_interrupts callback. So we can replace checking flag PHY_HAS_INTERRUPT with checking for these callbacks. This allows to remove this flag from a lot of driver configs, let's start with the Realtek driver. Heiner Kallweit (2): net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt net: phy: realtek: remove flag PHY_HAS_INTERRUPT from driver configs drivers/net/phy/phy_device.c | 5 +++-- drivers/net/phy/realtek.c| 7 --- 2 files changed, 3 insertions(+), 9 deletions(-) -- 2.19.1
[PATCH net-next] net: phy: improve struct phy_device member interrupts handling
As a heritage from the very early days of phylib member interrupts is
defined as u32 even though it's just a flag whether interrupts are
enabled. So we can change it to a bitfield member. In addition change
the code dealing with this member in a way that it's clear we're
dealing with a bool value.
Signed-off-by: Heiner Kallweit
---
Actually this member isn't needed at all and could be replaced with
a parameter in phy_driver->config_intr. But this would mean an API
change, maybe I come up with a proposal later.
---
drivers/net/phy/phy.c | 2 +-
include/linux/phy.h | 10 +-
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index dd5bff955..a5e6acfe6 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -115,7 +115,7 @@ static int phy_clear_interrupt(struct phy_device *phydev)
*
* Returns 0 on success or < 0 on error.
*/
-static int phy_config_interrupt(struct phy_device *phydev, u32 interrupts)
+static int phy_config_interrupt(struct phy_device *phydev, bool interrupts)
{
phydev->interrupts = interrupts;
if (phydev->drv->config_intr)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 5dd85c441..fc90af152 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -263,8 +263,8 @@ static inline struct mii_bus *devm_mdiobus_alloc(struct
device *dev)
void devm_mdiobus_free(struct device *dev, struct mii_bus *bus);
struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
-#define PHY_INTERRUPT_DISABLED 0x0
-#define PHY_INTERRUPT_ENABLED 0x8000
+#define PHY_INTERRUPT_DISABLED 0
+#define PHY_INTERRUPT_ENABLED 1
/* PHY state machine states:
*
@@ -410,6 +410,9 @@ struct phy_device {
/* The most recently read link state */
unsigned link:1;
+ /* Interrupts are enabled */
+ unsigned interrupts:1;
+
enum phy_state state;
u32 dev_flags;
@@ -425,9 +428,6 @@ struct phy_device {
int pause;
int asym_pause;
- /* Enabled Interrupts */
- u32 interrupts;
-
/* Union of PHY and Attached devices' supported modes */
/* See mii.h for more info */
u32 supported;
--
2.19.1
Re: [PATCH net-next 1/2] net: phy: use phy_id_mask value zero for exact match
On 08.11.2018 20:44, Florian Fainelli wrote:
> On 11/7/18 12:53 PM, Heiner Kallweit wrote:
>> A phy_id_mask value zero means every PHYID matches, therefore
>> value zero isn't used. So we can safely redefine the semantics
>> of value zero to mean "exact match". This allows to avoid some
>> boilerplate code in PHY driver configs.
>
> Having run recently into some ethtool quirkyness about how masks are
> supposed to be specified between ntuple/nfc, where the meaning of 0 is
> either don't care or match, I would rather we stick with the current
> behavior where every bit set to 0 is a don't care and bits set t 1 are not.
>
I agree that using a mask value 0 as either exact match or don't care
can be confusing. However I think that the advantages here outweigh
this confusion aspect.
- We get a meaningful default in case a driver author misses to set
the phy_id_mask.
- If a driver author wants to enforce an exact match, he has to do
nothing and can rely on the core. This avoids mistakes like in the
Realtek case where the driver author meant exact match but specified
something completely different.
- Avoid boilerplate code
> Maybe we can find a clever way with a macro to specify only the PHY OUI
> and compute a suitable mask automatically?
>
I don't think so. For Realtek each driver is specific even to a model
revision (therefore mask 0x). Same applies to intel-xway.
In broadcom.c we have masks 0xfff0, so for each model, independent
of revision number. There is no general rule.
Also we can't simply check for the first-bit-set to derive a mask.
>>
>> Signed-off-by: Heiner Kallweit
>> ---
>> drivers/net/phy/phy_device.c | 21 +++--
>> include/linux/phy.h | 2 +-
>> 2 files changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index ab33d1777..d165a2c82 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -483,15 +483,24 @@ static int phy_bus_match(struct device *dev, struct
>> device_driver *drv)
>> if (!(phydev->c45_ids.devices_in_package & (1 << i)))
>> continue;
>>
>> -if ((phydrv->phy_id & phydrv->phy_id_mask) ==
>> -(phydev->c45_ids.device_ids[i] &
>> - phydrv->phy_id_mask))
>> -return 1;
>> +if (!phydrv->phy_id_mask) {
>> +if (phydrv->phy_id ==
>> +phydev->c45_ids.device_ids[i])
>> +return 1;
>> +} else {
>> +if ((phydrv->phy_id & phydrv->phy_id_mask) ==
>> +(phydev->c45_ids.device_ids[i] &
>> + phydrv->phy_id_mask))
>> +return 1;
>> +}
>> }
>> return 0;
>> } else {
>> -return (phydrv->phy_id & phydrv->phy_id_mask) ==
>> -(phydev->phy_id & phydrv->phy_id_mask);
>> +if (!phydrv->phy_id_mask)
>> +return phydrv->phy_id == phydev->phy_id;
>> +else
>> +return (phydrv->phy_id & phydrv->phy_id_mask) ==
>> + (phydev->phy_id & phydrv->phy_id_mask);
>> }
>> }
>>
>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>> index 2090277ea..e30ca2fdd 100644
>> --- a/include/linux/phy.h
>> +++ b/include/linux/phy.h
>> @@ -500,7 +500,7 @@ struct phy_driver {
>> struct mdio_driver_common mdiodrv;
>> u32 phy_id;
>> char *name;
>> -u32 phy_id_mask;
>> +u32 phy_id_mask; /* value 0 means exact match */
>> const unsigned long * const features;
>> u32 flags;
>> const void *driver_data;
>>
>
>
Re: [PATCH net-next 2/2] net: phy: realtek: remove boilerplate code from driver configs
On 08.11.2018 19:37, Andrew Lunn wrote:
>> {
>> .phy_id = 0x8201,
>> .name = "RTL8201CP Ethernet",
>> -.phy_id_mask= 0x,
>> .features = PHY_BASIC_FEATURES,
>> .flags = PHY_HAS_INTERRUPT,
>> }, {
>> .phy_id = 0x001cc816,
>> .name = "RTL8201F Fast Ethernet",
>> -.phy_id_mask= 0x001f,
>
> Hi Heiner
>
> "RTL8201CP Ethernet" has a mask of 0x, where as all the others
> use 0x001f. Is this correct?
>
IMO none of the masks is correct. All of them should be 0x.
Nowadays the 32 bit PHYID is assembled from (MSB to LSB):
22 bits vendor OUI, 6 bit model number, 4 bit revision number
Just the old 8201 doesn't follow this scheme.
With the current masks, a PHYID 0x12348201 would be recognized as
Realtek 8201 too, what's obviously wrong.
> Andrew
>
Re: [PATCH net-next 0/5] net: phy: improve and simplify phylib state machine
On 07.11.2018 21:45, Heiner Kallweit wrote: > On 07.11.2018 21:21, Andrew Lunn wrote: >> On Wed, Nov 07, 2018 at 09:05:49PM +0100, Heiner Kallweit wrote: >>> On 07.11.2018 20:48, Andrew Lunn wrote: >>>> On Wed, Nov 07, 2018 at 08:41:52PM +0100, Heiner Kallweit wrote: >>>>> This patch series is based on two axioms: >>>>> >>>>> - During autoneg a PHY always reports the link being down >>>> >>>> Hi Heiner >>>> >>>> I think that is a risky assumption to make. >>>> >>> I wasn't sure initially too (found no clear rule in 802.3 clause 22) >>> and therefore asked around. Florian agrees to the assumption, >>> see here: https://www.spinics.net/lists/netdev/msg519242.html >>> >>> If a PHY reports the link as up then every user would assume that >>> data can be transferred. But that's not the case during aneg. >>> Therefore reporting the link as up during aneg wouldn't make sense. >> >> Hi Heiner >> >> If auto-neg has already been completed once before, i can see a lazy >> hardware designed not reporting link down, or at least, not until >> auto-neg actually fails. >> > "aneg finished" flag means that the aneg parameters in the register set > are valid. Once the link goes down that's not necessarily the case any > longer. E.g. some PHYs have an "auto speed down" feature and reduce > the speed to save power once they detect the link is down. > Of course I can not rule out that there are broken designs (or as you > stated more politely: lazy designs) out there. But in this case I assume > we would see issues already. And we would have to think about whether we > want to support such broken / lazy designs in phylib. > Had one more look at the changes to check what happens if "link up" and "aneg done" are not in sync. When link goes down the changes don't change current behavior. We just check the "link up" bit. When link goes up and aneg isn't finished yet, then we would report "link is up" earlier to userspace than we do now. If userspace starts some network activity based on the "link up" event then they may fail. But it really would be a major flaw if a PHY signals "link up" whilst it's not actually ready yet to transfer data. In case of such a broken design we would have issues with the current code already, at least if interrupts are used. The "link up" interrupt would cause the state machine to go to PHY_CHANGELINK which doesn't check for aneg status. >> And what about if link is down for too short a time for us to notice? >> I've seen some code fail because the kernel went off and did something >> else for too long, and a state change was missed. >> > This is a case we have already, independent of my change. > genphy_update_link() reads BMSR twice, thus ignoring potential latched > info about a temporary link failure. When polling phylib ignores > everything that happens between two poll intervals. > >>>> What happens if this assumption is incorrect? >>>> >>> Then we have to flush this patch series down the drain ;) >>> At least I would have to check in detail which parts need to be >>> changed. I clearly mention the assumptions so that every >>> reviewer can check whether he agrees. >> >> Thanks for doing that. I want to be happy this is safe, and not going >> to introduce regressions. >> >>Andrew >> >
[PATCH net-next 1/2] net: phy: use phy_id_mask value zero for exact match
A phy_id_mask value zero means every PHYID matches, therefore
value zero isn't used. So we can safely redefine the semantics
of value zero to mean "exact match". This allows to avoid some
boilerplate code in PHY driver configs.
Signed-off-by: Heiner Kallweit
---
drivers/net/phy/phy_device.c | 21 +++--
include/linux/phy.h | 2 +-
2 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index ab33d1777..d165a2c82 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -483,15 +483,24 @@ static int phy_bus_match(struct device *dev, struct
device_driver *drv)
if (!(phydev->c45_ids.devices_in_package & (1 << i)))
continue;
- if ((phydrv->phy_id & phydrv->phy_id_mask) ==
- (phydev->c45_ids.device_ids[i] &
-phydrv->phy_id_mask))
- return 1;
+ if (!phydrv->phy_id_mask) {
+ if (phydrv->phy_id ==
+ phydev->c45_ids.device_ids[i])
+ return 1;
+ } else {
+ if ((phydrv->phy_id & phydrv->phy_id_mask) ==
+ (phydev->c45_ids.device_ids[i] &
+phydrv->phy_id_mask))
+ return 1;
+ }
}
return 0;
} else {
- return (phydrv->phy_id & phydrv->phy_id_mask) ==
- (phydev->phy_id & phydrv->phy_id_mask);
+ if (!phydrv->phy_id_mask)
+ return phydrv->phy_id == phydev->phy_id;
+ else
+ return (phydrv->phy_id & phydrv->phy_id_mask) ==
+ (phydev->phy_id & phydrv->phy_id_mask);
}
}
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 2090277ea..e30ca2fdd 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -500,7 +500,7 @@ struct phy_driver {
struct mdio_driver_common mdiodrv;
u32 phy_id;
char *name;
- u32 phy_id_mask;
+ u32 phy_id_mask; /* value 0 means exact match */
const unsigned long * const features;
u32 flags;
const void *driver_data;
--
2.19.1
[PATCH net-next 2/2] net: phy: realtek: remove boilerplate code from driver configs
After a recent change phy_id_mask value 0 means "exact match". This
allows to remove setting phy_id_mask values from the driver configs.
Signed-off-by: Heiner Kallweit
---
drivers/net/phy/realtek.c | 9 -
1 file changed, 9 deletions(-)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 7b1c89b38..abff4cdc9 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -215,13 +215,11 @@ static struct phy_driver realtek_drvs[] = {
{
.phy_id = 0x8201,
.name = "RTL8201CP Ethernet",
- .phy_id_mask= 0x,
.features = PHY_BASIC_FEATURES,
.flags = PHY_HAS_INTERRUPT,
}, {
.phy_id = 0x001cc816,
.name = "RTL8201F Fast Ethernet",
- .phy_id_mask= 0x001f,
.features = PHY_BASIC_FEATURES,
.flags = PHY_HAS_INTERRUPT,
.ack_interrupt = _ack_interrupt,
@@ -233,7 +231,6 @@ static struct phy_driver realtek_drvs[] = {
}, {
.phy_id = 0x001cc910,
.name = "RTL8211 Gigabit Ethernet",
- .phy_id_mask= 0x001f,
.features = PHY_GBIT_FEATURES,
.config_aneg= rtl8211_config_aneg,
.read_mmd = _read_mmd_unsupported,
@@ -241,7 +238,6 @@ static struct phy_driver realtek_drvs[] = {
}, {
.phy_id = 0x001cc912,
.name = "RTL8211B Gigabit Ethernet",
- .phy_id_mask= 0x001f,
.features = PHY_GBIT_FEATURES,
.flags = PHY_HAS_INTERRUPT,
.ack_interrupt = _ack_interrupt,
@@ -253,7 +249,6 @@ static struct phy_driver realtek_drvs[] = {
}, {
.phy_id = 0x001cc913,
.name = "RTL8211C Gigabit Ethernet",
- .phy_id_mask= 0x001f,
.features = PHY_GBIT_FEATURES,
.config_init= rtl8211c_config_init,
.read_mmd = _read_mmd_unsupported,
@@ -261,7 +256,6 @@ static struct phy_driver realtek_drvs[] = {
}, {
.phy_id = 0x001cc914,
.name = "RTL8211DN Gigabit Ethernet",
- .phy_id_mask= 0x001f,
.features = PHY_GBIT_FEATURES,
.flags = PHY_HAS_INTERRUPT,
.ack_interrupt = rtl821x_ack_interrupt,
@@ -271,7 +265,6 @@ static struct phy_driver realtek_drvs[] = {
}, {
.phy_id = 0x001cc915,
.name = "RTL8211E Gigabit Ethernet",
- .phy_id_mask= 0x001f,
.features = PHY_GBIT_FEATURES,
.flags = PHY_HAS_INTERRUPT,
.ack_interrupt = _ack_interrupt,
@@ -281,7 +274,6 @@ static struct phy_driver realtek_drvs[] = {
}, {
.phy_id = 0x001cc916,
.name = "RTL8211F Gigabit Ethernet",
- .phy_id_mask= 0x001f,
.features = PHY_GBIT_FEATURES,
.flags = PHY_HAS_INTERRUPT,
.config_init= _config_init,
@@ -294,7 +286,6 @@ static struct phy_driver realtek_drvs[] = {
}, {
.phy_id = 0x001cc961,
.name = "RTL8366RB Gigabit Ethernet",
- .phy_id_mask= 0x001f,
.features = PHY_GBIT_FEATURES,
.flags = PHY_HAS_INTERRUPT,
.config_init= _config_init,
--
2.19.1
[PATCH net-next 0/2] net: phy: use phy_id_mask value zero for exact match
A phy_id_mask value zero means every PHYID matches, therefore value zero isn't used. So we can safely redefine the semantics of value zero to mean "exact match". This allows to avoid some boilerplate code in PHY driver configs. Realtek PHY driver is the first user of this change. Heiner Kallweit (2): net: phy: use phy_id_mask value zero for exact match net: phy: realtek: remove boilerplate code from PHY driver definitions drivers/net/phy/phy_device.c | 21 +++-- drivers/net/phy/realtek.c| 9 - include/linux/phy.h | 2 +- 3 files changed, 16 insertions(+), 16 deletions(-) -- 2.19.1
Re: [PATCH net-next 0/5] net: phy: improve and simplify phylib state machine
On 07.11.2018 21:21, Andrew Lunn wrote: > On Wed, Nov 07, 2018 at 09:05:49PM +0100, Heiner Kallweit wrote: >> On 07.11.2018 20:48, Andrew Lunn wrote: >>> On Wed, Nov 07, 2018 at 08:41:52PM +0100, Heiner Kallweit wrote: >>>> This patch series is based on two axioms: >>>> >>>> - During autoneg a PHY always reports the link being down >>> >>> Hi Heiner >>> >>> I think that is a risky assumption to make. >>> >> I wasn't sure initially too (found no clear rule in 802.3 clause 22) >> and therefore asked around. Florian agrees to the assumption, >> see here: https://www.spinics.net/lists/netdev/msg519242.html >> >> If a PHY reports the link as up then every user would assume that >> data can be transferred. But that's not the case during aneg. >> Therefore reporting the link as up during aneg wouldn't make sense. > > Hi Heiner > > If auto-neg has already been completed once before, i can see a lazy > hardware designed not reporting link down, or at least, not until > auto-neg actually fails. > "aneg finished" flag means that the aneg parameters in the register set are valid. Once the link goes down that's not necessarily the case any longer. E.g. some PHYs have an "auto speed down" feature and reduce the speed to save power once they detect the link is down. Of course I can not rule out that there are broken designs (or as you stated more politely: lazy designs) out there. But in this case I assume we would see issues already. And we would have to think about whether we want to support such broken / lazy designs in phylib. > And what about if link is down for too short a time for us to notice? > I've seen some code fail because the kernel went off and did something > else for too long, and a state change was missed. > This is a case we have already, independent of my change. genphy_update_link() reads BMSR twice, thus ignoring potential latched info about a temporary link failure. When polling phylib ignores everything that happens between two poll intervals. >>> What happens if this assumption is incorrect? >>> >> Then we have to flush this patch series down the drain ;) >> At least I would have to check in detail which parts need to be >> changed. I clearly mention the assumptions so that every >> reviewer can check whether he agrees. > > Thanks for doing that. I want to be happy this is safe, and not going > to introduce regressions. > >Andrew >
Re: [PATCH net-next 0/5] net: phy: improve and simplify phylib state machine
On 07.11.2018 20:48, Andrew Lunn wrote: > On Wed, Nov 07, 2018 at 08:41:52PM +0100, Heiner Kallweit wrote: >> This patch series is based on two axioms: >> >> - During autoneg a PHY always reports the link being down > > Hi Heiner > > I think that is a risky assumption to make. > I wasn't sure initially too (found no clear rule in 802.3 clause 22) and therefore asked around. Florian agrees to the assumption, see here: https://www.spinics.net/lists/netdev/msg519242.html If a PHY reports the link as up then every user would assume that data can be transferred. But that's not the case during aneg. Therefore reporting the link as up during aneg wouldn't make sense. > What happens if this assumption is incorrect? > Then we have to flush this patch series down the drain ;) At least I would have to check in detail which parts need to be changed. I clearly mention the assumptions so that every reviewer can check whether he agrees. > Andrew > Heiner
[PATCH net-next 4/5] net: phy: remove state PHY_AN
After the recent changes in the state machine state PHY_AN isn't used
any longer and can be removed.
Signed-off-by: Heiner Kallweit
---
drivers/net/phy/phy.c | 27 ---
include/linux/phy.h | 19 +--
2 files changed, 1 insertion(+), 45 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 87ed00030..226824804 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -50,7 +50,6 @@ static const char *phy_state_to_str(enum phy_state st)
PHY_STATE_STR(READY)
PHY_STATE_STR(PENDING)
PHY_STATE_STR(UP)
- PHY_STATE_STR(AN)
PHY_STATE_STR(RUNNING)
PHY_STATE_STR(NOLINK)
PHY_STATE_STR(FORCING)
@@ -944,32 +943,6 @@ void phy_state_machine(struct work_struct *work)
case PHY_UP:
needs_aneg = true;
- phydev->link_timeout = PHY_AN_TIMEOUT;
-
- break;
- case PHY_AN:
- err = phy_read_status(phydev);
- if (err < 0)
- break;
-
- /* If the link is down, give up on negotiation for now */
- if (!phydev->link) {
- phydev->state = PHY_NOLINK;
- phy_link_down(phydev, true);
- break;
- }
-
- /* Check if negotiation is done. Break if there's an error */
- err = phy_aneg_done(phydev);
- if (err < 0)
- break;
-
- /* If AN is done, we're running */
- if (err > 0) {
- phydev->state = PHY_RUNNING;
- phy_link_up(phydev);
- } else if (0 == phydev->link_timeout--)
- needs_aneg = true;
break;
case PHY_NOLINK:
if (!phy_polling_mode(phydev))
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 9e4d49ef4..2090277ea 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -178,7 +178,6 @@ static inline const char *phy_modes(phy_interface_t
interface)
#define PHY_INIT_TIMEOUT 10
#define PHY_STATE_TIME 1
#define PHY_FORCE_TIMEOUT 10
-#define PHY_AN_TIMEOUT 10
#define PHY_MAX_ADDR 32
@@ -297,24 +296,10 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int
addr);
*
* UP: The PHY and attached device are ready to do work.
* Interrupts should be started here.
- * - timer moves to AN
- *
- * AN: The PHY is currently negotiating the link state. Link is
- * therefore down for now. phy_timer will set this state when it
- * detects the state is UP. config_aneg will set this state
- * whenever called with phydev->autoneg set to AUTONEG_ENABLE.
- * - If autonegotiation finishes, but there's no link, it sets
- * the state to NOLINK.
- * - If aneg finishes with link, it sets the state to RUNNING,
- * and calls adjust_link
- * - If autonegotiation did not finish after an arbitrary amount
- * of time, autonegotiation should be tried again if the PHY
- * supports "magic" autonegotiation (back to AN)
- * - If it didn't finish, and no magic_aneg, move to FORCING.
+ * - timer moves to NOLINK or RUNNING
*
* NOLINK: PHY is up, but not currently plugged in.
* - If the timer notes that the link comes back, we move to RUNNING
- * - config_aneg moves to AN
* - phy_stop moves to HALTED
*
* FORCING: PHY is being configured with forced settings
@@ -329,7 +314,6 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int
addr);
* link state is polled every other cycle of this state machine,
* which makes it every other second)
* - irq will set CHANGELINK
- * - config_aneg will set AN
* - phy_stop moves to HALTED
*
* CHANGELINK: PHY experienced a change in link state
@@ -353,7 +337,6 @@ enum phy_state {
PHY_READY,
PHY_PENDING,
PHY_UP,
- PHY_AN,
PHY_RUNNING,
PHY_NOLINK,
PHY_FORCING,
--
2.19.1
[PATCH net-next 2/5] net: phy: remove useless check in state machine case PHY_RESUMING
If aneg isn't finished yet then the PHY reports the link as down.
There's no benefit in setting the state to PHY_AN because the next
state machine run would set the status to PHY_NOLINK anyway (except
in the meantime aneg has been finished and link is up). Therefore
we can set the state to PHY_RUNNING or PHY_NOLINK directly.
In addition change the do_carrier parameter in phy_link_down() to true.
If carrier was marked as up before (what should never be the case because
PHY was in state PHY_HALTED before) then we should mark it as down now.
Signed-off-by: Heiner Kallweit
---
drivers/net/phy/phy.c | 13 +
1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 87c6d304c..14dffa0da 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1022,17 +1022,6 @@ void phy_state_machine(struct work_struct *work)
}
break;
case PHY_RESUMING:
- if (AUTONEG_ENABLE == phydev->autoneg) {
- err = phy_aneg_done(phydev);
- if (err < 0) {
- break;
- } else if (!err) {
- phydev->state = PHY_AN;
- phydev->link_timeout = PHY_AN_TIMEOUT;
- break;
- }
- }
-
err = phy_read_status(phydev);
if (err)
break;
@@ -1042,7 +1031,7 @@ void phy_state_machine(struct work_struct *work)
phy_link_up(phydev);
} else {
phydev->state = PHY_NOLINK;
- phy_link_down(phydev, false);
+ phy_link_down(phydev, true);
}
break;
}
--
2.19.1
[PATCH net-next 3/5] net: phy: add phy_check_link_status
In few places in the state machine the state is set to PHY_RUNNING or
PHY_NOLINK after doing a phy_read_status(). So factor this out to
phy_check_link_status().
First use it in phy_start_aneg(): By setting the state to PHY_RUNNING
or PHY_NOLINK directly we can remove the code to handle the case that
we're using interrupts and aneg was finished already.
Definition of phy_link_up and phy_link_down needs to be moved because
they are called in the new function.
Signed-off-by: Heiner Kallweit
---
drivers/net/phy/phy.c | 70 ---
1 file changed, 40 insertions(+), 30 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 14dffa0da..87ed00030 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -62,6 +62,17 @@ static const char *phy_state_to_str(enum phy_state st)
return NULL;
}
+static void phy_link_up(struct phy_device *phydev)
+{
+ phydev->phy_link_change(phydev, true, true);
+ phy_led_trigger_change_speed(phydev);
+}
+
+static void phy_link_down(struct phy_device *phydev, bool do_carrier)
+{
+ phydev->phy_link_change(phydev, false, do_carrier);
+ phy_led_trigger_change_speed(phydev);
+}
/**
* phy_print_status - Convenience function to print out the current phy status
@@ -493,6 +504,34 @@ static int phy_config_aneg(struct phy_device *phydev)
return genphy_config_aneg(phydev);
}
+/**
+ * phy_check_link_status - check link status and set state accordingly
+ * @phydev: the phy_device struct
+ *
+ * Description: Check for link and whether autoneg was triggered / is running
+ * and set state accordingly
+ */
+static int phy_check_link_status(struct phy_device *phydev)
+{
+ int err;
+
+ WARN_ON(!mutex_is_locked(>lock));
+
+ err = phy_read_status(phydev);
+ if (err)
+ return err;
+
+ if (phydev->link && phydev->state != PHY_RUNNING) {
+ phydev->state = PHY_RUNNING;
+ phy_link_up(phydev);
+ } else if (!phydev->link && phydev->state != PHY_NOLINK) {
+ phydev->state = PHY_NOLINK;
+ phy_link_down(phydev, true);
+ }
+
+ return 0;
+}
+
/**
* phy_start_aneg - start auto-negotiation for this PHY device
* @phydev: the phy_device struct
@@ -504,7 +543,6 @@ static int phy_config_aneg(struct phy_device *phydev)
*/
int phy_start_aneg(struct phy_device *phydev)
{
- bool trigger = 0;
int err;
if (!phydev->drv)
@@ -524,32 +562,16 @@ int phy_start_aneg(struct phy_device *phydev)
if (phydev->state != PHY_HALTED) {
if (AUTONEG_ENABLE == phydev->autoneg) {
- phydev->state = PHY_AN;
- phydev->link_timeout = PHY_AN_TIMEOUT;
+ err = phy_check_link_status(phydev);
} else {
phydev->state = PHY_FORCING;
phydev->link_timeout = PHY_FORCE_TIMEOUT;
}
}
- /* Re-schedule a PHY state machine to check PHY status because
-* negotiation may already be done and aneg interrupt may not be
-* generated.
-*/
- if (!phy_polling_mode(phydev) && phydev->state == PHY_AN) {
- err = phy_aneg_done(phydev);
- if (err > 0) {
- trigger = true;
- err = 0;
- }
- }
-
out_unlock:
mutex_unlock(>lock);
- if (trigger)
- phy_trigger_machine(phydev);
-
return err;
}
EXPORT_SYMBOL(phy_start_aneg);
@@ -893,18 +915,6 @@ void phy_start(struct phy_device *phydev)
}
EXPORT_SYMBOL(phy_start);
-static void phy_link_up(struct phy_device *phydev)
-{
- phydev->phy_link_change(phydev, true, true);
- phy_led_trigger_change_speed(phydev);
-}
-
-static void phy_link_down(struct phy_device *phydev, bool do_carrier)
-{
- phydev->phy_link_change(phydev, false, do_carrier);
- phy_led_trigger_change_speed(phydev);
-}
-
/**
* phy_state_machine - Handle the state machine
* @work: work_struct that describes the work to be done
--
2.19.1
[PATCH net-next 5/5] net: phy: use phy_check_link_status in more places in the state machine
Use phy_check_link_status in more places in the state machine.
Signed-off-by: Heiner Kallweit
---
drivers/net/phy/phy.c | 53 ---
1 file changed, 5 insertions(+), 48 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 226824804..dd5bff955 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -945,17 +945,13 @@ void phy_state_machine(struct work_struct *work)
break;
case PHY_NOLINK:
+ case PHY_RUNNING:
if (!phy_polling_mode(phydev))
break;
-
- err = phy_read_status(phydev);
- if (err)
- break;
-
- if (phydev->link) {
- phydev->state = PHY_RUNNING;
- phy_link_up(phydev);
- }
+ /* fall through */
+ case PHY_CHANGELINK:
+ case PHY_RESUMING:
+ err = phy_check_link_status(phydev);
break;
case PHY_FORCING:
err = genphy_update_link(phydev);
@@ -971,32 +967,6 @@ void phy_state_machine(struct work_struct *work)
phy_link_down(phydev, false);
}
break;
- case PHY_RUNNING:
- if (!phy_polling_mode(phydev))
- break;
-
- err = phy_read_status(phydev);
- if (err)
- break;
-
- if (!phydev->link) {
- phydev->state = PHY_NOLINK;
- phy_link_down(phydev, true);
- }
- break;
- case PHY_CHANGELINK:
- err = phy_read_status(phydev);
- if (err)
- break;
-
- if (phydev->link) {
- phydev->state = PHY_RUNNING;
- phy_link_up(phydev);
- } else {
- phydev->state = PHY_NOLINK;
- phy_link_down(phydev, true);
- }
- break;
case PHY_HALTED:
if (phydev->link) {
phydev->link = 0;
@@ -1004,19 +974,6 @@ void phy_state_machine(struct work_struct *work)
do_suspend = true;
}
break;
- case PHY_RESUMING:
- err = phy_read_status(phydev);
- if (err)
- break;
-
- if (phydev->link) {
- phydev->state = PHY_RUNNING;
- phy_link_up(phydev);
- } else {
- phydev->state = PHY_NOLINK;
- phy_link_down(phydev, true);
- }
- break;
}
mutex_unlock(>lock);
--
2.19.1
[PATCH net-next 1/5] net: phy: remove useless check in state machine case PHY_NOLINK
If aneg is enabled and the PHY reports the link as up then definitely
aneg finished successfully. Therefore this check is useless and
can be removed.
Signed-off-by: Heiner Kallweit
---
drivers/net/phy/phy.c | 11 ---
1 file changed, 11 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 476578746..87c6d304c 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -970,17 +970,6 @@ void phy_state_machine(struct work_struct *work)
break;
if (phydev->link) {
- if (AUTONEG_ENABLE == phydev->autoneg) {
- err = phy_aneg_done(phydev);
- if (err < 0)
- break;
-
- if (!err) {
- phydev->state = PHY_AN;
- phydev->link_timeout = PHY_AN_TIMEOUT;
- break;
- }
- }
phydev->state = PHY_RUNNING;
phy_link_up(phydev);
}
--
2.19.1
[PATCH net-next 0/5] net: phy: improve and simplify phylib state machine
This patch series is based on two axioms: - During autoneg a PHY always reports the link being down - Info in clause 22/45 registers doesn't allow to differentiate between these two states: 1. Link is physically down 2. A link partner is connected and PHY is autonegotiating In both cases "link up" and "aneg finished" bits aren't set. One consequence is that having separate states PHY_NOLINK and PHY_AN isn't needed. By using these two axioms the state machine can be significantly simplified. Heiner Kallweit (5): net: phy: remove useless check in state machine case PHY_NOLINK net: phy: remove useless check in state machine case PHY_RESUMING net: phy: add phy_check_link_status net: phy: remove state PHY_AN net: phy: use phy_check_link_status in more places in the state machine drivers/net/phy/phy.c | 172 +++--- include/linux/phy.h | 19 + 2 files changed, 46 insertions(+), 145 deletions(-) -- 2.19.1
[PATCH net-next] net: phy: realtek: load driver for all PHYs with a Realtek OUI
Instead of listing every single PHYID, load the driver for every PHYID
with a Realtek OUI, independent of model number and revision.
This patch also improves two further aspects:
- constify realtek_tbl[]
- the mask should have been 0x instead of 0x001f so far,
by masking out some bits a PHY from another vendor could have been
matched
Signed-off-by: Heiner Kallweit
---
drivers/net/phy/realtek.c | 11 ++-
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 271e8adc3..7b1c89b38 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -305,15 +305,8 @@ static struct phy_driver realtek_drvs[] = {
module_phy_driver(realtek_drvs);
-static struct mdio_device_id __maybe_unused realtek_tbl[] = {
- { 0x001cc816, 0x001f },
- { 0x001cc910, 0x001f },
- { 0x001cc912, 0x001f },
- { 0x001cc913, 0x001f },
- { 0x001cc914, 0x001f },
- { 0x001cc915, 0x001f },
- { 0x001cc916, 0x001f },
- { 0x001cc961, 0x001f },
+static const struct mdio_device_id __maybe_unused realtek_tbl[] = {
+ { 0x001cc800, GENMASK(31, 10) },
{ }
};
--
2.19.1
[PATCH net-next] net: phy: make phy_trigger_machine static
phy_trigger_machine() is used in phy.c only, so we can make it static.
Signed-off-by: Heiner Kallweit
---
drivers/net/phy/phy.c | 33 -
include/linux/phy.h | 1 -
2 files changed, 12 insertions(+), 22 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 1d73ac330..476578746 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -467,6 +467,18 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq
*ifr, int cmd)
}
EXPORT_SYMBOL(phy_mii_ioctl);
+static void phy_queue_state_machine(struct phy_device *phydev,
+ unsigned int secs)
+{
+ mod_delayed_work(system_power_efficient_wq, >state_queue,
+secs * HZ);
+}
+
+static void phy_trigger_machine(struct phy_device *phydev)
+{
+ phy_queue_state_machine(phydev, 0);
+}
+
static int phy_config_aneg(struct phy_device *phydev)
{
if (phydev->drv->config_aneg)
@@ -620,13 +632,6 @@ int phy_speed_up(struct phy_device *phydev)
}
EXPORT_SYMBOL_GPL(phy_speed_up);
-static void phy_queue_state_machine(struct phy_device *phydev,
- unsigned int secs)
-{
- mod_delayed_work(system_power_efficient_wq, >state_queue,
-secs * HZ);
-}
-
/**
* phy_start_machine - start PHY state machine tracking
* @phydev: the phy_device struct
@@ -643,20 +648,6 @@ void phy_start_machine(struct phy_device *phydev)
}
EXPORT_SYMBOL_GPL(phy_start_machine);
-/**
- * phy_trigger_machine - trigger the state machine to run
- *
- * @phydev: the phy_device struct
- *
- * Description: There has been a change in state which requires that the
- * state machine runs.
- */
-
-void phy_trigger_machine(struct phy_device *phydev)
-{
- phy_queue_state_machine(phydev, 0);
-}
-
/**
* phy_stop_machine - stop the PHY state machine tracking
* @phydev: target phy_device struct
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 3ea87f774..9e4d49ef4 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1054,7 +1054,6 @@ void phy_change_work(struct work_struct *work);
void phy_mac_interrupt(struct phy_device *phydev);
void phy_start_machine(struct phy_device *phydev);
void phy_stop_machine(struct phy_device *phydev);
-void phy_trigger_machine(struct phy_device *phydev);
int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd *cmd);
void phy_ethtool_ksettings_get(struct phy_device *phydev,
struct ethtool_link_ksettings *cmd);
--
2.19.1
[PATCH net] r8169: fix broken Wake-on-LAN from S5 (poweroff)
It was reported that WoL from S5 is broken (WoL from S3 works) and the
analysis showed that during system shutdown the network interface was
brought down already when the actual kernel shutdown started.
Therefore netif_running() returned false and as a consequence the PHY
was suspended. Obviously WoL wasn't working then.
To fix this the original patch needs to be effectively reverted.
A side effect is that when normally bringing down the interface and
WoL is enabled the PHY will remain powered on (like it was before the
original patch).
Fixes: fe87bef01f9b ("r8169: don't check WoL when powering down PHY and
interface is down")
Reported-by: Neil MacLeod
Signed-off-by: Heiner Kallweit
---
drivers/net/ethernet/realtek/r8169.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index 0d8070adc..987fedeee 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4162,10 +4162,15 @@ static void rtl_wol_suspend_quirk(struct
rtl8169_private *tp)
static bool rtl_wol_pll_power_down(struct rtl8169_private *tp)
{
- if (!netif_running(tp->dev) || !__rtl8169_get_wol(tp))
+ struct phy_device *phydev;
+
+ if (!__rtl8169_get_wol(tp))
return false;
- phy_speed_down(tp->dev->phydev, false);
+ /* phydev may not be attached to netdevice */
+ phydev = mdiobus_get_phy(tp->mii_bus, 0);
+
+ phy_speed_down(phydev, false);
rtl_wol_suspend_quirk(tp);
return true;
--
2.19.1
Re: [PATCH] r8169: Add new device ID support
On 24.10.2018 23:22, David Miller wrote: > From: Shawn Lin > Date: Wed, 24 Oct 2018 09:46:47 +0800 > >> It's found my r8169 ethernet card at hand has a device ID >> of 0x which wasn't on the list of rtl8169_pci_tbl. Add >> a new entry to make it work: >> >> [2.165785] r8169 Gigabit Ethernet driver 2.3LK-NAPI loaded >> [2.165863] r8169 :01:00.0: enabling device ( -> 0003) >> [2.167110] r8169 :01:00.0 eth0: RTL8168c/8111c at 0xff80089be000, >> 00:e0:4c:21:00:17, XID 1c4000c0 IRQ 208 >> [2.167128] r8169 :01:00.0 eth0: jumbo features [frames: 6128 >> bytes, tx checksumming: ko] >> >> [root@rk1808:/]# lspci >> 00:00.0 Class 0604: 1d87:1808 >> 01:00.0 Class 0200: 10ec: >> >> Signed-off-by: Shawn Lin > > I'm stil not terribly confident in this change, a device ID of zero is > really unusual. > > Heiner, what do you think? > A PCI device ID of zero definitely is a mistake of the card vendor. Or maybe the card was just a sample and not meant to retail? If some vendor of cards with a different Realtek network chip makes the same mistake, then we're in trouble. I don't think we should accept this risk just to support a broken ancient card. This card most likely is at least 10 years old, and that we get the first report only now seems to indicate that it's not something affecting a lot of people. The reporter found a way to make the card work on his system, so I don't see a need for any further action.
r8169: changed rx buffer alignment requirement
Hi Eric,
when working on the r8169 driver I came across an old patch from you:
6f0333b8fde4 ("r8169: use 50% less ram for RX ring")
As part of this patch the alignment requirement for rx buffers was
silently changed from 8 to 16 bytes. Can you remember (well, after
eight years) why you did this? In the chip datasheet I find only
8 bytes mentioned as requirement.
Regards,
Heiner
[PATCH net-next] r8169: add support for Byte Queue Limits
From: Florian Westphal
This patch is basically a resubmit of 1e918876853a ("r8169: add support
for Byte Queue Limits") which was reverted later. The problems causing
the revert seem to have been fixed in the meantime.
Only change to the original patch is that the call to
netdev_reset_queue was moved to rtl8169_tx_clear.
The Tested-by refers to a system using the RTL8168evl chip version.
Signed-off-by: Florian Westphal
Signed-off-by: Heiner Kallweit
Tested-by: Holger Hoffstätte
---
drivers/net/ethernet/realtek/r8169.c | 18 +-
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index 114bd9e54..006b0aa8c 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -5851,6 +5851,7 @@ static void rtl8169_tx_clear(struct rtl8169_private *tp)
{
rtl8169_tx_clear_range(tp, tp->dirty_tx, NUM_TX_DESC);
tp->cur_tx = tp->dirty_tx = 0;
+ netdev_reset_queue(tp->dev);
}
static void rtl_reset_work(struct rtl8169_private *tp)
@@ -6153,6 +6154,8 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
txd->opts2 = cpu_to_le32(opts[1]);
+ netdev_sent_queue(dev, skb->len);
+
skb_tx_timestamp(skb);
/* Force memory writes to complete before releasing descriptor */
@@ -6251,7 +6254,7 @@ static void rtl8169_pcierr_interrupt(struct net_device
*dev)
static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
{
- unsigned int dirty_tx, tx_left;
+ unsigned int dirty_tx, tx_left, bytes_compl = 0, pkts_compl = 0;
dirty_tx = tp->dirty_tx;
smp_rmb();
@@ -6275,10 +6278,8 @@ static void rtl_tx(struct net_device *dev, struct
rtl8169_private *tp)
rtl8169_unmap_tx_skb(tp_to_dev(tp), tx_skb,
tp->TxDescArray + entry);
if (status & LastFrag) {
- u64_stats_update_begin(>tx_stats.syncp);
- tp->tx_stats.packets++;
- tp->tx_stats.bytes += tx_skb->skb->len;
- u64_stats_update_end(>tx_stats.syncp);
+ pkts_compl++;
+ bytes_compl += tx_skb->skb->len;
dev_consume_skb_any(tx_skb->skb);
tx_skb->skb = NULL;
}
@@ -6287,6 +6288,13 @@ static void rtl_tx(struct net_device *dev, struct
rtl8169_private *tp)
}
if (tp->dirty_tx != dirty_tx) {
+ netdev_completed_queue(dev, pkts_compl, bytes_compl);
+
+ u64_stats_update_begin(>tx_stats.syncp);
+ tp->tx_stats.packets += pkts_compl;
+ tp->tx_stats.bytes += bytes_compl;
+ u64_stats_update_end(>tx_stats.syncp);
+
tp->dirty_tx = dirty_tx;
/* Sync with rtl8169_start_xmit:
* - publish dirty_tx ring index (write barrier)
--
2.19.1
[PATCH net-next] r8169: handle all interrupt events in the hard irq handler
Having a separate "slow event" handler isn't needed because all
interrupt events trigger asynchronous activity. And in case of SYSErr
we have bigger problems than performance anyway.
This patch also allows to get rid of acking interrupt events in the
NAPI poll callback.
Signed-off-by: Heiner Kallweit
---
This patch will apply to net-next only after 6b839b6cf9ea ("r8169: fix
NAPI handling under high load") was merged from net to net-next.
---
drivers/net/ethernet/realtek/r8169.c | 65
1 file changed, 19 insertions(+), 46 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index 03d5fb7c7..3075b493d 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -631,7 +631,6 @@ struct rtl8169_tc_offsets {
enum rtl_flag {
RTL_FLAG_TASK_ENABLED = 0,
- RTL_FLAG_TASK_SLOW_PENDING,
RTL_FLAG_TASK_RESET_PENDING,
RTL_FLAG_MAX
};
@@ -6460,42 +6459,29 @@ static irqreturn_t rtl8169_interrupt(int irq, void
*dev_instance)
if (status == 0x || !(status & (RTL_EVENT_NAPI | tp->event_slow)))
return IRQ_NONE;
- rtl_irq_disable(tp);
- napi_schedule_irqoff(>napi);
-
- return IRQ_HANDLED;
-}
-
-/*
- * Workqueue context.
- */
-static void rtl_slow_event_work(struct rtl8169_private *tp)
-{
- struct net_device *dev = tp->dev;
- u16 status;
+ if (unlikely(status & SYSErr)) {
+ rtl8169_pcierr_interrupt(tp->dev);
+ goto out;
+ }
- status = rtl_get_events(tp) & tp->event_slow;
- rtl_ack_events(tp, status);
+ if (status & LinkChg)
+ phy_mac_interrupt(tp->dev->phydev);
- if (unlikely(status & RxFIFOOver)) {
- switch (tp->mac_version) {
- /* Work around for rx fifo overflow */
- case RTL_GIGA_MAC_VER_11:
- netif_stop_queue(dev);
- /* XXX - Hack alert. See rtl_task(). */
- set_bit(RTL_FLAG_TASK_RESET_PENDING, tp->wk.flags);
- default:
- break;
- }
+ if (unlikely(status & RxFIFOOver &&
+ tp->mac_version == RTL_GIGA_MAC_VER_11)) {
+ netif_stop_queue(tp->dev);
+ /* XXX - Hack alert. See rtl_task(). */
+ set_bit(RTL_FLAG_TASK_RESET_PENDING, tp->wk.flags);
}
- if (unlikely(status & SYSErr))
- rtl8169_pcierr_interrupt(dev);
-
- if (status & LinkChg)
- phy_mac_interrupt(dev->phydev);
+ if (status & RTL_EVENT_NAPI) {
+ rtl_irq_disable(tp);
+ napi_schedule_irqoff(>napi);
+ }
+out:
+ rtl_ack_events(tp, status);
- rtl_irq_enable_all(tp);
+ return IRQ_HANDLED;
}
static void rtl_task(struct work_struct *work)
@@ -6504,8 +6490,6 @@ static void rtl_task(struct work_struct *work)
int bitnr;
void (*action)(struct rtl8169_private *);
} rtl_work[] = {
- /* XXX - keep rtl_slow_event_work() as first element. */
- { RTL_FLAG_TASK_SLOW_PENDING, rtl_slow_event_work },
{ RTL_FLAG_TASK_RESET_PENDING, rtl_reset_work },
};
struct rtl8169_private *tp =
@@ -6535,27 +6519,16 @@ static int rtl8169_poll(struct napi_struct *napi, int
budget)
{
struct rtl8169_private *tp = container_of(napi, struct rtl8169_private,
napi);
struct net_device *dev = tp->dev;
- u16 enable_mask = RTL_EVENT_NAPI | tp->event_slow;
int work_done;
- u16 status;
-
- status = rtl_get_events(tp);
- rtl_ack_events(tp, status & ~tp->event_slow);
work_done = rtl_rx(dev, tp, (u32) budget);
rtl_tx(dev, tp);
- if (status & tp->event_slow) {
- enable_mask &= ~tp->event_slow;
-
- rtl_schedule_task(tp, RTL_FLAG_TASK_SLOW_PENDING);
- }
-
if (work_done < budget) {
napi_complete_done(napi, work_done);
- rtl_irq_enable(tp, enable_mask);
+ rtl_irq_enable_all(tp);
mmiowb();
}
--
2.19.1
Re: [PATCH v2 net] r8169: fix NAPI handling under high load
On 18.10.2018 20:02, Eric Dumazet wrote:
>
>
> On 10/18/2018 10:56 AM, Heiner Kallweit wrote:
>> rtl_rx() and rtl_tx() are called only if the respective bits are set
>> in the interrupt status register. Under high load NAPI may not be
>> able to process all data (work_done == budget) and it will schedule
>> subsequent calls to the poll callback.
>> rtl_ack_events() however resets the bits in the interrupt status
>> register, therefore subsequent calls to rtl8169_poll() won't call
>> rtl_rx() and rtl_tx() - chip interrupts are still disabled.
>>
>> Fix this by calling rtl_rx() and rtl_tx() independent of the bits
>> set in the interrupt status register. Both functions will detect
>> if there's nothing to do for them.
>>
>> Fixes: da78dbff2e05 ("r8169: remove work from irq handler.")
>> Signed-off-by: Heiner Kallweit
>> ---
>> v2:
>> - added "Fixes" tag
>> ---
>> drivers/net/ethernet/realtek/r8169.c | 8 +++-
>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/realtek/r8169.c
>> b/drivers/net/ethernet/realtek/r8169.c
>> index 8c4f49adc..7caf3b7e9 100644
>> --- a/drivers/net/ethernet/realtek/r8169.c
>> +++ b/drivers/net/ethernet/realtek/r8169.c
>> @@ -6528,17 +6528,15 @@ static int rtl8169_poll(struct napi_struct *napi,
>> int budget)
>> struct rtl8169_private *tp = container_of(napi, struct rtl8169_private,
>> napi);
>> struct net_device *dev = tp->dev;
>> u16 enable_mask = RTL_EVENT_NAPI | tp->event_slow;
>> -int work_done= 0;
>> +int work_done;
>> u16 status;
>>
>> status = rtl_get_events(tp);
>> rtl_ack_events(tp, status & ~tp->event_slow);
>>
>> -if (status & RTL_EVENT_NAPI_RX)
>> -work_done = rtl_rx(dev, tp, (u32) budget);
>> +work_done = rtl_rx(dev, tp, (u32) budget);
>>
>> -if (status & RTL_EVENT_NAPI_TX)
>> -rtl_tx(dev, tp);
>> +rtl_tx(dev, tp);
>>
>> if (status & tp->event_slow) {
>> enable_mask &= ~tp->event_slow;
>>
>
> One has to wonder why rtl8169_poll(), which might be called in a loop under
> DOS,
> has to call rtl_ack_events() ?
>
> Should not this be called one time from hard irq handler instead ?
>
Yes, it should. I have a patch in my tree including this, in addition it
moves everything from rtl_slow_event_work() to the hard irq handler.
However, due to recent changes, this patch may not apply cleanly to
older kernel versions.
Calling rtl_ack_events() in the poll callback isn't nice but not directly
a bug, so I'd prefer to submit this to net-next.
>
[PATCH v2 net] r8169: fix NAPI handling under high load
rtl_rx() and rtl_tx() are called only if the respective bits are set
in the interrupt status register. Under high load NAPI may not be
able to process all data (work_done == budget) and it will schedule
subsequent calls to the poll callback.
rtl_ack_events() however resets the bits in the interrupt status
register, therefore subsequent calls to rtl8169_poll() won't call
rtl_rx() and rtl_tx() - chip interrupts are still disabled.
Fix this by calling rtl_rx() and rtl_tx() independent of the bits
set in the interrupt status register. Both functions will detect
if there's nothing to do for them.
Fixes: da78dbff2e05 ("r8169: remove work from irq handler.")
Signed-off-by: Heiner Kallweit
---
v2:
- added "Fixes" tag
---
drivers/net/ethernet/realtek/r8169.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index 8c4f49adc..7caf3b7e9 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -6528,17 +6528,15 @@ static int rtl8169_poll(struct napi_struct *napi, int
budget)
struct rtl8169_private *tp = container_of(napi, struct rtl8169_private,
napi);
struct net_device *dev = tp->dev;
u16 enable_mask = RTL_EVENT_NAPI | tp->event_slow;
- int work_done= 0;
+ int work_done;
u16 status;
status = rtl_get_events(tp);
rtl_ack_events(tp, status & ~tp->event_slow);
- if (status & RTL_EVENT_NAPI_RX)
- work_done = rtl_rx(dev, tp, (u32) budget);
+ work_done = rtl_rx(dev, tp, (u32) budget);
- if (status & RTL_EVENT_NAPI_TX)
- rtl_tx(dev, tp);
+ rtl_tx(dev, tp);
if (status & tp->event_slow) {
enable_mask &= ~tp->event_slow;
--
2.19.1
Re: [PATCH net] r8169: fix NAPI handling under high load
On 18.10.2018 07:58, Jonathan Woithe wrote: > On Thu, Oct 18, 2018 at 01:30:51AM +0200, Francois Romieu wrote: >> Holger Hoffstätte : >> [...] >>> I continued to use the BQL patch in my private tree after it was reverted >>> and also had occasional timeouts, but *only* after I started playing >>> with ethtool to change offload settings. Without offloads or the BQL patch >>> everything has been rock-solid since then. >>> The other weird problem was that timeouts would occur on an otherwise >>> *completely idle* system. Since that occasionally borked my NFS server >>> over night I ultimately removed BQL as well. Rock-solid since then. >> >> The bug will induce delayed rx processing when a spike of "load" is >> followed by an idle period. > > If this is the case, I wonder whether this bug might also be the cause of > the long reception delays we've observed at times when a period of high > network load is followed by almost nothing[1]. That thread[2] details the > investigations subsequently done. A git bisect showed that commit > da78dbff2e05630921c551dbbc70a4b7981a8fff was the origin of the misbehaviour > we were observing. > > We still see the problem when we test with recent kernels. It would be > great if the underlying problem has now been identified. > > I can possibly scrape some hardware together to test any proposed fix under > our workload if there was interest. > Proposed fix is here: https://patchwork.ozlabs.org/patch/985014/ Would be good if you could test it. Thanks! Heiner > Regards > jonathan > > [1] https://marc.info/?l=linux-netdev=136281333207734=2 > [2] https://marc.info/?t=13628133952=1=2 >
Re: [PATCH net] r8169: fix NAPI handling under high load
On 18.10.2018 07:21, David Miller wrote:
> From: Francois Romieu
> Date: Thu, 18 Oct 2018 01:30:45 +0200
>
>> Heiner Kallweit :
>> [...]
>>> This issue has been there more or less forever (at least it exists in
>>> 3.16 already), so I can't provide a "Fixes" tag.
>>
>> Hardly forever. It fixes da78dbff2e05630921c551dbbc70a4b7981a8fff.
>
> I don't see exactly how that can be true.
>
> That commit didn't change the parts of the NAPI poll processing which
> are relevant here, mainly the guarding of the RX and TX work using
> the status bits which are cleared.
>
AFAICS Francois is right and patch da78dbff2e05 ("r8169: remove work
from irq handler") introduced the guarding of RX and TX work.
I just checked back to 3.16 as oldest LTS kernel version.
> Maybe I'm missing something? If so, indeed it would be nice to add
> a proper Fixes: tag here.
>
Shall I submit a v2 including the Fixes line?
> Thanks!
>
Re: [PATCH net] r8169: fix NAPI handling under high load
On 17.10.2018 21:11, Holger Hoffstätte wrote:
> On 10/17/18 20:12, Heiner Kallweit wrote:
>> On 16.10.2018 23:17, Holger Hoffstätte wrote:
>>> On 10/16/18 22:37, Heiner Kallweit wrote:
>>>> rtl_rx() and rtl_tx() are called only if the respective bits are set
>>>> in the interrupt status register. Under high load NAPI may not be
>>>> able to process all data (work_done == budget) and it will schedule
>>>> subsequent calls to the poll callback.
>>>> rtl_ack_events() however resets the bits in the interrupt status
>>>> register, therefore subsequent calls to rtl8169_poll() won't call
>>>> rtl_rx() and rtl_tx() - chip interrupts are still disabled.
>>>
>>> Very interesting! Could this be the reason for the mysterious
>>> hangs & resets we experienced when enabling BQL for r8169?
>>> They happened more often with TSO/GSO enabled and several people
>>> attempted to fix those hangs unsuccessfully; it was later reverted
>>> and has been since then (#87cda7cb43).
>>> If this bug has been there "forever" it might be tempting to
>>> re-apply BQL and see what happens. Any chance you could give that
>>> a try? I'll gladly test patches, just like I'll run this one.
>>>
>> After reading through the old mail threads regarding BQL on r8169
>> I don't think the fix here is related.
>> It seems that BQL on r8169 worked fine for most people, just one
>> had problems on one of his systems. I assume the issue was specific
>
> I continued to use the BQL patch in my private tree after it was reverted
> and also had occasional timeouts, but *only* after I started playing
> with ethtool to change offload settings. Without offloads or the BQL patch
> everything has been rock-solid since then.
> The other weird problem was that timeouts would occur on an otherwise
> *completely idle* system. Since that occasionally borked my NFS server
> over night I ultimately removed BQL as well. Rock-solid since then.
>
>> I will apply the old BQL patch and see how it's on my system
>> (with GRO and SG enabled).
>
> I don't think it still applies cleanly, but if you cook up an updated
> version I'll gladly test it.
>
> Thanks! :)
> Holger
>
Good to know. What's your kernel version and RTL8168 chip version?
Regarding the chip version the dmesg line with the XID would be relevant.
Below is the slightly modified original BQL patch, I just moved the call
to netdev_reset_queue(). This patch applies at least to latest linux-next.
My test system:
- RTL8168evl
- latest linux-next
- BQL patch applied
- SG/GRO enabled:
rx-checksumming: on
tx-checksumming: on
tx-checksum-ipv4: on
tx-checksum-ip-generic: off [fixed]
tx-checksum-ipv6: on
tx-checksum-fcoe-crc: off [fixed]
tx-checksum-sctp: off [fixed]
scatter-gather: on
tx-scatter-gather: on
tx-scatter-gather-fraglist: off [fixed]
tcp-segmentation-offload: on
tx-tcp-segmentation: on
tx-tcp-ecn-segmentation: off [fixed]
tx-tcp-mangleid-segmentation: on
tx-tcp6-segmentation: on
udp-fragmentation-offload: off
generic-segmentation-offload: on
generic-receive-offload: on
large-receive-offload: off [fixed]
rx-vlan-offload: on
tx-vlan-offload: on
I briefly tested normal operation and did some tests with iperf3.
Everything looks good so far.
---
drivers/net/ethernet/realtek/r8169.c | 18 +-
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index 0d8070adc..e236b46b8 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -5852,6 +5852,7 @@ static void rtl8169_tx_clear(struct rtl8169_private *tp)
{
rtl8169_tx_clear_range(tp, tp->dirty_tx, NUM_TX_DESC);
tp->cur_tx = tp->dirty_tx = 0;
+ netdev_reset_queue(tp->dev);
}
static void rtl_reset_work(struct rtl8169_private *tp)
@@ -6154,6 +6155,8 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
txd->opts2 = cpu_to_le32(opts[1]);
+ netdev_sent_queue(dev, skb->len);
+
skb_tx_timestamp(skb);
/* Force memory writes to complete before releasing descriptor */
@@ -6252,7 +6255,7 @@ static void rtl8169_pcierr_interrupt(struct net_device
*dev)
static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
{
- unsigned int dirty_tx, tx_left;
+ unsigned int dirty_tx, tx_left, bytes_compl = 0, pkts_compl = 0;
dirty_tx = tp->dirty_tx;
smp_rmb();
@@ -6276,10 +6279,8 @@ static void rtl_tx(struct net_device *dev, struct
rtl8169_private *tp)
rtl8169_unmap_tx_skb(tp_to_dev(tp), tx_skb,
Fwd: Re: [PATCH net] r8169: fix NAPI handling under high load
Tomas, more than three years back you reported network problems after BQL was added to the r8169 driver. Due to this the change was reverted. Now the discussion to add BQL popped up again. You mentioned that the issue exists on one of your systems only. Therefore it could be an issue specific to a particular chip version. I'd be interested in the chip version of the affected system. You linked to another similar report, there the chip version was: r8169 :10:00.0 eth0: RTL8168c/8111c at 0xf813, 00:e0:4c:68:48:d2, XID 1c4000c0 IRQ 29 In case you still have the affected system or at least the old dmesg logs, I'd appreciate if you could let me know the quoted line from dmesg output. Thanks a lot, Heiner Forwarded Message Subject: Re: [PATCH net] r8169: fix NAPI handling under high load Date: Wed, 17 Oct 2018 20:12:48 +0200 From: Heiner Kallweit To: Holger Hoffstätte , David Miller , Realtek linux nic maintainers CC: netdev@vger.kernel.org On 16.10.2018 23:17, Holger Hoffstätte wrote: > On 10/16/18 22:37, Heiner Kallweit wrote: >> rtl_rx() and rtl_tx() are called only if the respective bits are set >> in the interrupt status register. Under high load NAPI may not be >> able to process all data (work_done == budget) and it will schedule >> subsequent calls to the poll callback. >> rtl_ack_events() however resets the bits in the interrupt status >> register, therefore subsequent calls to rtl8169_poll() won't call >> rtl_rx() and rtl_tx() - chip interrupts are still disabled. > > Very interesting! Could this be the reason for the mysterious > hangs & resets we experienced when enabling BQL for r8169? > They happened more often with TSO/GSO enabled and several people > attempted to fix those hangs unsuccessfully; it was later reverted > and has been since then (#87cda7cb43). > If this bug has been there "forever" it might be tempting to > re-apply BQL and see what happens. Any chance you could give that > a try? I'll gladly test patches, just like I'll run this one. > After reading through the old mail threads regarding BQL on r8169 I don't think the fix here is related. It seems that BQL on r8169 worked fine for most people, just one had problems on one of his systems. I assume the issue was specific to one chip version. From the ~ 50 chip versions supported by r8169 more or less each one requires its own quirks. If we're lucky the chip-version-specific issue has been fixed in the meantime and we can simply apply the old BQL patch again. If it turns out that certain chip versions can't be used with BQL, then we can disable the feature for these chip versions instead of removing the feature completely. I will apply the old BQL patch and see how it's on my system (with GRO and SG enabled). > cheers > Holger > Heiner
Re: [PATCH net] r8169: fix NAPI handling under high load
On 16.10.2018 23:17, Holger Hoffstätte wrote: > On 10/16/18 22:37, Heiner Kallweit wrote: >> rtl_rx() and rtl_tx() are called only if the respective bits are set >> in the interrupt status register. Under high load NAPI may not be >> able to process all data (work_done == budget) and it will schedule >> subsequent calls to the poll callback. >> rtl_ack_events() however resets the bits in the interrupt status >> register, therefore subsequent calls to rtl8169_poll() won't call >> rtl_rx() and rtl_tx() - chip interrupts are still disabled. > > Very interesting! Could this be the reason for the mysterious > hangs & resets we experienced when enabling BQL for r8169? > They happened more often with TSO/GSO enabled and several people > attempted to fix those hangs unsuccessfully; it was later reverted > and has been since then (#87cda7cb43). > If this bug has been there "forever" it might be tempting to > re-apply BQL and see what happens. Any chance you could give that > a try? I'll gladly test patches, just like I'll run this one. > After reading through the old mail threads regarding BQL on r8169 I don't think the fix here is related. It seems that BQL on r8169 worked fine for most people, just one had problems on one of his systems. I assume the issue was specific to one chip version. From the ~ 50 chip versions supported by r8169 more or less each one requires its own quirks. If we're lucky the chip-version-specific issue has been fixed in the meantime and we can simply apply the old BQL patch again. If it turns out that certain chip versions can't be used with BQL, then we can disable the feature for these chip versions instead of removing the feature completely. I will apply the old BQL patch and see how it's on my system (with GRO and SG enabled). > cheers > Holger > Heiner
[PATCH net] r8169: fix NAPI handling under high load
rtl_rx() and rtl_tx() are called only if the respective bits are set
in the interrupt status register. Under high load NAPI may not be
able to process all data (work_done == budget) and it will schedule
subsequent calls to the poll callback.
rtl_ack_events() however resets the bits in the interrupt status
register, therefore subsequent calls to rtl8169_poll() won't call
rtl_rx() and rtl_tx() - chip interrupts are still disabled.
Fix this by calling rtl_rx() and rtl_tx() independent of the bits
set in the interrupt status register. Both functions will detect
if there's nothing to do for them.
This issue has been there more or less forever (at least it exists in
3.16 already), so I can't provide a "Fixes" tag.
Signed-off-by: Heiner Kallweit
---
drivers/net/ethernet/realtek/r8169.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index 8c4f49adc..7caf3b7e9 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -6528,17 +6528,15 @@ static int rtl8169_poll(struct napi_struct *napi, int
budget)
struct rtl8169_private *tp = container_of(napi, struct rtl8169_private,
napi);
struct net_device *dev = tp->dev;
u16 enable_mask = RTL_EVENT_NAPI | tp->event_slow;
- int work_done= 0;
+ int work_done;
u16 status;
status = rtl_get_events(tp);
rtl_ack_events(tp, status & ~tp->event_slow);
- if (status & RTL_EVENT_NAPI_RX)
- work_done = rtl_rx(dev, tp, (u32) budget);
+ work_done = rtl_rx(dev, tp, (u32) budget);
- if (status & RTL_EVENT_NAPI_TX)
- rtl_tx(dev, tp);
+ rtl_tx(dev, tp);
if (status & tp->event_slow) {
enable_mask &= ~tp->event_slow;
--
2.19.1
[PATCH net] r8169: re-enable MSI-X on RTL8168g
Similar to d49c88d7677b ("r8169: Enable MSI-X on RTL8106e") after
e9d0ba506ea8 ("PCI: Reprogram bridge prefetch registers on resume")
we can safely assume that this also fixes the root cause of
the issue worked around by 7c53a722459c ("r8169: don't use MSI-X on
RTL8168g"). So let's revert it.
Fixes: 7c53a722459c ("r8169: don't use MSI-X on RTL8168g")
Signed-off-by: Heiner Kallweit
---
drivers/net/ethernet/realtek/r8169.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index f4df367fb..28184b984 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -7098,11 +7098,6 @@ static int rtl_alloc_irq(struct rtl8169_private *tp)
RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~MSIEnable);
RTL_W8(tp, Cfg9346, Cfg9346_Lock);
flags = PCI_IRQ_LEGACY;
- } else if (tp->mac_version == RTL_GIGA_MAC_VER_40) {
- /* This version was reported to have issues with resume
-* from suspend when using MSI-X
-*/
- flags = PCI_IRQ_LEGACY | PCI_IRQ_MSI;
} else {
flags = PCI_IRQ_ALL_TYPES;
}
--
2.19.1
[PATCH net-next] net: phy: merge phy_start_aneg and phy_start_aneg_priv
After commit 9f2959b6b52d ("net: phy: improve handling delayed work")
the sync parameter isn't needed any longer in phy_start_aneg_priv().
This allows to merge phy_start_aneg() and phy_start_aneg_priv().
Signed-off-by: Heiner Kallweit
---
drivers/net/phy/phy.c | 21 +++--
1 file changed, 3 insertions(+), 18 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index d03bdbbd1..1d73ac330 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -482,16 +482,15 @@ static int phy_config_aneg(struct phy_device *phydev)
}
/**
- * phy_start_aneg_priv - start auto-negotiation for this PHY device
+ * phy_start_aneg - start auto-negotiation for this PHY device
* @phydev: the phy_device struct
- * @sync: indicate whether we should wait for the workqueue cancelation
*
* Description: Sanitizes the settings (if we're not autonegotiating
* them), and then calls the driver's config_aneg function.
* If the PHYCONTROL Layer is operating, we change the state to
* reflect the beginning of Auto-negotiation or forcing.
*/
-static int phy_start_aneg_priv(struct phy_device *phydev, bool sync)
+int phy_start_aneg(struct phy_device *phydev)
{
bool trigger = 0;
int err;
@@ -541,20 +540,6 @@ static int phy_start_aneg_priv(struct phy_device *phydev,
bool sync)
return err;
}
-
-/**
- * phy_start_aneg - start auto-negotiation for this PHY device
- * @phydev: the phy_device struct
- *
- * Description: Sanitizes the settings (if we're not autonegotiating
- * them), and then calls the driver's config_aneg function.
- * If the PHYCONTROL Layer is operating, we change the state to
- * reflect the beginning of Auto-negotiation or forcing.
- */
-int phy_start_aneg(struct phy_device *phydev)
-{
- return phy_start_aneg_priv(phydev, true);
-}
EXPORT_SYMBOL(phy_start_aneg);
static int phy_poll_aneg_done(struct phy_device *phydev)
@@ -1085,7 +1070,7 @@ void phy_state_machine(struct work_struct *work)
mutex_unlock(>lock);
if (needs_aneg)
- err = phy_start_aneg_priv(phydev, false);
+ err = phy_start_aneg(phydev);
else if (do_suspend)
phy_suspend(phydev);
--
2.19.1
[PATCH net-next] r8169: remove unneeded call to netif_stop_queue in rtl8169_net_suspend
netif_device_detach() stops all tx queues already, so we don't need this call. Signed-off-by: Heiner Kallweit --- drivers/net/ethernet/realtek/r8169.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c index e6e1790f4..a78be5937 100644 --- a/drivers/net/ethernet/realtek/r8169.c +++ b/drivers/net/ethernet/realtek/r8169.c @@ -6825,7 +6825,6 @@ static void rtl8169_net_suspend(struct net_device *dev) phy_stop(dev->phydev); netif_device_detach(dev); - netif_stop_queue(dev); rtl_lock_work(tp); napi_disable(>napi); -- 2.19.1
[PATCH net-next] r8169: simplify rtl8169_set_magic_reg
Simplify this function, no functional change intended.
Signed-off-by: Heiner Kallweit
---
drivers/net/ethernet/realtek/r8169.c | 32 +++-
1 file changed, 12 insertions(+), 20 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index 7d3f671e1..e6e1790f4 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4553,27 +4553,19 @@ static void rtl_set_rx_tx_desc_registers(struct
rtl8169_private *tp)
static void rtl8169_set_magic_reg(struct rtl8169_private *tp, unsigned
mac_version)
{
- static const struct rtl_cfg2_info {
- u32 mac_version;
- u32 clk;
- u32 val;
- } cfg2_info [] = {
- { RTL_GIGA_MAC_VER_05, PCI_Clock_33MHz, 0x000fff00 }, // 8110SCd
- { RTL_GIGA_MAC_VER_05, PCI_Clock_66MHz, 0x000f },
- { RTL_GIGA_MAC_VER_06, PCI_Clock_33MHz, 0x0000 }, // 8110SCe
- { RTL_GIGA_MAC_VER_06, PCI_Clock_66MHz, 0x00ff }
- };
- const struct rtl_cfg2_info *p = cfg2_info;
- unsigned int i;
- u32 clk;
+ u32 val;
- clk = RTL_R8(tp, Config2) & PCI_Clock_66MHz;
- for (i = 0; i < ARRAY_SIZE(cfg2_info); i++, p++) {
- if ((p->mac_version == mac_version) && (p->clk == clk)) {
- RTL_W32(tp, 0x7c, p->val);
- break;
- }
- }
+ if (tp->mac_version == RTL_GIGA_MAC_VER_05)
+ val = 0x000fff00;
+ else if (tp->mac_version == RTL_GIGA_MAC_VER_06)
+ val = 0x0000;
+ else
+ return;
+
+ if (RTL_R8(tp, Config2) & PCI_Clock_66MHz)
+ val |= 0xff;
+
+ RTL_W32(tp, 0x7c, val);
}
static void rtl_set_rx_mode(struct net_device *dev)
--
2.19.1
[PATCH net-next 1/2] net: phy: improve handling of PHY_RUNNING in state machine
Handling of state PHY_RUNNING seems to be more complex than it needs
to be. If not polling, then we don't have to do anything, we'll
receive an interrupt and go to state PHY_CHANGELINK once the link
goes down. If polling and link is down, we don't have to go the
extra mile over PHY_CHANGELINK and call phy_read_status() again
but can set status PHY_NOLINK directly.
Signed-off-by: Heiner Kallweit
---
drivers/net/phy/phy.c | 29 +
1 file changed, 9 insertions(+), 20 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 704428211..696955d38 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -941,7 +941,6 @@ void phy_state_machine(struct work_struct *work)
bool needs_aneg = false, do_suspend = false;
enum phy_state old_state;
int err = 0;
- int old_link;
mutex_lock(>lock);
@@ -1025,26 +1024,16 @@ void phy_state_machine(struct work_struct *work)
}
break;
case PHY_RUNNING:
- /* Only register a CHANGE if we are polling and link changed
-* since latest checking.
-*/
- if (phy_polling_mode(phydev)) {
- old_link = phydev->link;
- err = phy_read_status(phydev);
- if (err)
- break;
+ if (!phy_polling_mode(phydev))
+ break;
- if (old_link != phydev->link)
- phydev->state = PHY_CHANGELINK;
- }
- /*
-* Failsafe: check that nobody set phydev->link=0 between two
-* poll cycles, otherwise we won't leave RUNNING state as long
-* as link remains down.
-*/
- if (!phydev->link && phydev->state == PHY_RUNNING) {
- phydev->state = PHY_CHANGELINK;
- phydev_err(phydev, "no link in PHY_RUNNING\n");
+ err = phy_read_status(phydev);
+ if (err)
+ break;
+
+ if (!phydev->link) {
+ phydev->state = PHY_NOLINK;
+ phy_link_down(phydev, true);
}
break;
case PHY_CHANGELINK:
--
2.19.1
[PATCH net-next 2/2] net: phy: simplify handling of PHY_RESUMING in state machine
Simplify code for handling state PHY_RESUMING, no functional change
intended.
Signed-off-by: Heiner Kallweit
---
drivers/net/phy/phy.c | 43 ++-
1 file changed, 14 insertions(+), 29 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 696955d38..d03bdbbd1 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1059,41 +1059,26 @@ void phy_state_machine(struct work_struct *work)
case PHY_RESUMING:
if (AUTONEG_ENABLE == phydev->autoneg) {
err = phy_aneg_done(phydev);
- if (err < 0)
+ if (err < 0) {
break;
-
- /* err > 0 if AN is done.
-* Otherwise, it's 0, and we're still waiting for AN
-*/
- if (err > 0) {
- err = phy_read_status(phydev);
- if (err)
- break;
-
- if (phydev->link) {
- phydev->state = PHY_RUNNING;
- phy_link_up(phydev);
- } else {
- phydev->state = PHY_NOLINK;
- phy_link_down(phydev, false);
- }
- } else {
+ } else if (!err) {
phydev->state = PHY_AN;
phydev->link_timeout = PHY_AN_TIMEOUT;
- }
- } else {
- err = phy_read_status(phydev);
- if (err)
break;
-
- if (phydev->link) {
- phydev->state = PHY_RUNNING;
- phy_link_up(phydev);
- } else {
- phydev->state = PHY_NOLINK;
- phy_link_down(phydev, false);
}
}
+
+ err = phy_read_status(phydev);
+ if (err)
+ break;
+
+ if (phydev->link) {
+ phydev->state = PHY_RUNNING;
+ phy_link_up(phydev);
+ } else {
+ phydev->state = PHY_NOLINK;
+ phy_link_down(phydev, false);
+ }
break;
}
--
2.19.1
[PATCH net-next 0/2] net: phy: improve and simplify state machine
Improve / simplify handling of states PHY_RUNNING and PHY_RESUMING in phylib state machine. Heiner Kallweit (2): net: phy: improve handling of PHY_RUNNING in state machine net: phy: simplify handling of PHY_RESUMING in state machine drivers/net/phy/phy.c | 72 ++- 1 file changed, 23 insertions(+), 49 deletions(-) -- 2.19.1
[PATCH net-next] net: phy: trigger state machine immediately in phy_start_machine
When starting the state machine there may be work to be done
immediately, e.g. if the initial state is PHY_UP then the state
machine may trigger an autonegotiation. Having said that I see no need
to wait a second until the state machine is run first time.
Signed-off-by: Heiner Kallweit
---
drivers/net/phy/phy.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 14509a890..704428211 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -654,7 +654,7 @@ static void phy_queue_state_machine(struct phy_device
*phydev,
*/
void phy_start_machine(struct phy_device *phydev)
{
- phy_queue_state_machine(phydev, 1);
+ phy_trigger_machine(phydev);
}
EXPORT_SYMBOL_GPL(phy_start_machine);
--
2.19.1
Re: [PATCH net-next v2] net: core: change bool members of struct net_device to bitfield members
On 09.10.2018 17:20, David Ahern wrote: > On 10/8/18 2:17 PM, Heiner Kallweit wrote: >> bool is good as parameter type or function return type, but if used >> for struct members it consumes more memory than needed. >> Changing the bool members of struct net_device to bitfield members >> allows to decrease the memory footprint of this struct. > > What does pahole show for the size of the struct before and after? I > suspect you have not really changed the size and certainly not the > actual memory allocated. > > Thanks for the hint to use pahole. Indeed we gain nothing, so there's no justification for this patch. before: /* size: 2496, cachelines: 39, members: 116 */ /* sum members: 2396, holes: 8, sum holes: 80 */ /* padding: 20 */ /* paddings: 4, sum paddings: 19 */ /* bit_padding: 31 bits */ after: /* size: 2496, cachelines: 39, members: 116 */ /* sum members: 2394, holes: 8, sum holes: 82 */ /* bit holes: 1, sum bit holes: 8 bits */ /* padding: 20 */ /* paddings: 4, sum paddings: 19 */ /* bit_padding: 27 bits */ The biggest hole is here, because _tx is annotated to be cacheline-aligned. struct hlist_node index_hlist; /* 88816 */ /* XXX 56 bytes hole, try to pack */ /* --- cacheline 15 boundary (960 bytes) --- */ struct netdev_queue * _tx; /* 960 8 */ Reordering the struct members to fill the holes could be a little tricky and could have side effects because it may make a performance difference whether certain members are in one cacheline or not. And whether it's worth to spend this effort (incl. the related risks) just to save a few bytes (also considering that typically we have quite few instances of struct net_device)?
Re: [PATCH net-next] net: core: change bool members of struct net_device to bitfield members
On 08.10.2018 23:20, Stephen Hemminger wrote: > On Mon, 8 Oct 2018 22:00:51 +0200 > Heiner Kallweit wrote: > >> * >> + * @uc_promisc:Counter that indicates promiscuous mode >> + * has been enabled due to the need to listen to >> + * additional unicast addresses in a device that >> + * does not implement ndo_set_rx_mode() >> + * > > I see you just moved the pre-existing comment, but it the comment > looks incorrect. uc_promisc is not a counter but a flag. A counter would > have more than two states normally. > Right. A v2 fixing the comment has been submitted already.
[PATCH net-next v2] net: core: change bool members of struct net_device to bitfield members
bool is good as parameter type or function return type, but if used
for struct members it consumes more memory than needed.
Changing the bool members of struct net_device to bitfield members
allows to decrease the memory footprint of this struct.
Signed-off-by: Heiner Kallweit
---
v2:
- Change Counter to Flag in description of uc_promisc
---
include/linux/netdevice.h | 24 +---
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 76603ee13..3d7b8df2e 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1651,10 +1651,6 @@ enum netdev_priv_flags {
* @dev_port: Used to differentiate devices that share
* the same function
* @addr_list_lock:XXX: need comments on this one
- * @uc_promisc:Counter that indicates promiscuous mode
- * has been enabled due to the need to listen to
- * additional unicast addresses in a device that
- * does not implement ndo_set_rx_mode()
* @uc:unicast mac addresses
* @mc:multicast mac addresses
* @dev_addrs: list of device hw addresses
@@ -1714,11 +1710,9 @@ enum netdev_priv_flags {
* @link_watch_list: XXX: need comments on this one
*
* @reg_state: Register/unregister state machine
- * @dismantle: Device is going to be freed
* @rtnl_link_state: This enum represents the phases of creating
* a new link
*
- * @needs_free_netdev: Should unregister perform free_netdev?
* @priv_destructor: Called from unregister
* @npinfo:XXX: need comments on this one
* @nd_net:Network namespace this network device is inside
@@ -1758,6 +1752,15 @@ enum netdev_priv_flags {
* @qdisc_tx_busylock: lockdep class annotating Qdisc->busylock spinlock
* @qdisc_running_key: lockdep class annotating Qdisc->running seqcount
*
+ * @uc_promisc:Flag that indicates promiscuous mode
+ * has been enabled due to the need to listen to
+ * additional unicast addresses in a device that
+ * does not implement ndo_set_rx_mode()
+ *
+ * @dismantle: Device is going to be freed
+ *
+ * @needs_free_netdev: Should unregister perform free_netdev?
+ *
* @proto_down:protocol port state information can be sent to the
* switch driver and used to set the phys state of the
* switch port.
@@ -1879,7 +1882,6 @@ struct net_device {
unsigned short dev_port;
spinlock_t addr_list_lock;
unsigned char name_assign_type;
- booluc_promisc;
struct netdev_hw_addr_list uc;
struct netdev_hw_addr_list mc;
struct netdev_hw_addr_list dev_addrs;
@@ -1986,14 +1988,11 @@ struct net_device {
NETREG_DUMMY,/* dummy device for NAPI poll */
} reg_state:8;
- bool dismantle;
-
enum {
RTNL_LINK_INITIALIZED,
RTNL_LINK_INITIALIZING,
} rtnl_link_state:16;
- bool needs_free_netdev;
void (*priv_destructor)(struct net_device *dev);
#ifdef CONFIG_NETPOLL
@@ -2046,7 +2045,10 @@ struct net_device {
struct sfp_bus *sfp_bus;
struct lock_class_key *qdisc_tx_busylock;
struct lock_class_key *qdisc_running_key;
- boolproto_down;
+ unsigneduc_promisc:1;
+ unsigneddismantle:1;
+ unsignedneeds_free_netdev:1;
+ unsignedproto_down:1;
unsignedwol_enabled:1;
};
#define to_net_dev(d) container_of(d, struct net_device, dev)
--
2.19.1
Re: [PATCH net-next] net: core: change bool members of struct net_device to bitfield members
On 08.10.2018 22:07, Randy Dunlap wrote:
> On 10/8/18 1:00 PM, Heiner Kallweit wrote:
>> bool is good as parameter type or function return type, but if used
>> for struct members it consumes more memory than needed.
>> Changing the bool members of struct net_device to bitfield members
>> allows to decrease the memory footprint of this struct.
>>
>> Signed-off-by: Heiner Kallweit
>> ---
>> include/linux/netdevice.h | 24 +---
>> 1 file changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 76603ee13..3d7b8df2e 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -1651,10 +1651,6 @@ enum netdev_priv_flags {
>> * @dev_port: Used to differentiate devices that share
>> * the same function
>> * @addr_list_lock:XXX: need comments on this one
>> - * @uc_promisc:Counter that indicates promiscuous mode
>> - * has been enabled due to the need to listen to
>> - * additional unicast addresses in a device that
>> - * does not implement ndo_set_rx_mode()
>> * @uc:unicast mac addresses
>> * @mc:multicast mac addresses
>> * @dev_addrs: list of device hw addresses
>> @@ -1714,11 +1710,9 @@ enum netdev_priv_flags {
>> * @link_watch_list: XXX: need comments on this one
>> *
>> * @reg_state: Register/unregister state machine
>> - * @dismantle: Device is going to be freed
>> * @rtnl_link_state: This enum represents the phases of creating
>> * a new link
>> *
>> - * @needs_free_netdev: Should unregister perform free_netdev?
>> * @priv_destructor: Called from unregister
>> * @npinfo:XXX: need comments on this one
>> * @nd_net:Network namespace this network device is inside
>> @@ -1758,6 +1752,15 @@ enum netdev_priv_flags {
>> * @qdisc_tx_busylock: lockdep class annotating Qdisc->busylock spinlock
>> * @qdisc_running_key: lockdep class annotating Qdisc->running seqcount
>> *
>> + * @uc_promisc:Counter that indicates promiscuous mode
>> + * has been enabled due to the need to listen to
>> + * additional unicast addresses in a device that
>> + * does not implement ndo_set_rx_mode()
>
> Hi,
>
> I see that all you did is copy/paste that text (above), but I wouldn't call
> a single bit a [1-bit] Counter.
>
I stumbled across this comment too. Neither a bool member nor a 1-bit
bitfield member should be called a counter. I kept the original comment,
but I'm totally fine with changing Counter -> Flag and will provide a v2.
> thanks,
>
[PATCH net-next] net: core: change bool members of struct net_device to bitfield members
bool is good as parameter type or function return type, but if used
for struct members it consumes more memory than needed.
Changing the bool members of struct net_device to bitfield members
allows to decrease the memory footprint of this struct.
Signed-off-by: Heiner Kallweit
---
include/linux/netdevice.h | 24 +---
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 76603ee13..3d7b8df2e 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1651,10 +1651,6 @@ enum netdev_priv_flags {
* @dev_port: Used to differentiate devices that share
* the same function
* @addr_list_lock:XXX: need comments on this one
- * @uc_promisc:Counter that indicates promiscuous mode
- * has been enabled due to the need to listen to
- * additional unicast addresses in a device that
- * does not implement ndo_set_rx_mode()
* @uc:unicast mac addresses
* @mc:multicast mac addresses
* @dev_addrs: list of device hw addresses
@@ -1714,11 +1710,9 @@ enum netdev_priv_flags {
* @link_watch_list: XXX: need comments on this one
*
* @reg_state: Register/unregister state machine
- * @dismantle: Device is going to be freed
* @rtnl_link_state: This enum represents the phases of creating
* a new link
*
- * @needs_free_netdev: Should unregister perform free_netdev?
* @priv_destructor: Called from unregister
* @npinfo:XXX: need comments on this one
* @nd_net:Network namespace this network device is inside
@@ -1758,6 +1752,15 @@ enum netdev_priv_flags {
* @qdisc_tx_busylock: lockdep class annotating Qdisc->busylock spinlock
* @qdisc_running_key: lockdep class annotating Qdisc->running seqcount
*
+ * @uc_promisc:Counter that indicates promiscuous mode
+ * has been enabled due to the need to listen to
+ * additional unicast addresses in a device that
+ * does not implement ndo_set_rx_mode()
+ *
+ * @dismantle: Device is going to be freed
+ *
+ * @needs_free_netdev: Should unregister perform free_netdev?
+ *
* @proto_down:protocol port state information can be sent to the
* switch driver and used to set the phys state of the
* switch port.
@@ -1879,7 +1882,6 @@ struct net_device {
unsigned short dev_port;
spinlock_t addr_list_lock;
unsigned char name_assign_type;
- booluc_promisc;
struct netdev_hw_addr_list uc;
struct netdev_hw_addr_list mc;
struct netdev_hw_addr_list dev_addrs;
@@ -1986,14 +1988,11 @@ struct net_device {
NETREG_DUMMY,/* dummy device for NAPI poll */
} reg_state:8;
- bool dismantle;
-
enum {
RTNL_LINK_INITIALIZED,
RTNL_LINK_INITIALIZING,
} rtnl_link_state:16;
- bool needs_free_netdev;
void (*priv_destructor)(struct net_device *dev);
#ifdef CONFIG_NETPOLL
@@ -2046,7 +2045,10 @@ struct net_device {
struct sfp_bus *sfp_bus;
struct lock_class_key *qdisc_tx_busylock;
struct lock_class_key *qdisc_running_key;
- boolproto_down;
+ unsigneduc_promisc:1;
+ unsigneddismantle:1;
+ unsignedneeds_free_netdev:1;
+ unsignedproto_down:1;
unsignedwol_enabled:1;
};
#define to_net_dev(d) container_of(d, struct net_device, dev)
--
2.19.1
