* Re: [Patch net-next] mlx5: use RCU lock in mlx5_eq_cq_get()
From: Cong Wang @ 2019-02-06 22:51 UTC (permalink / raw)
To: Saeed Mahameed; +Cc: netdev@vger.kernel.org, Tariq Toukan
In-Reply-To: <90bd3088771ab2be083eac9772d488bc3ea8763b.camel@mellanox.com>
On Wed, Feb 6, 2019 at 9:40 AM Saeed Mahameed <saeedm@mellanox.com> wrote:
>
> On Wed, 2019-02-06 at 09:35 -0800, Saeed Mahameed wrote:
> > On Tue, 2019-02-05 at 16:35 -0800, Cong Wang wrote:
> > > mlx5_eq_cq_get() is called in IRQ handler, the spinlock inside
> > > gets a lot of contentions when we test some heavy workload
> > > with 60 RX queues and 80 CPU's, and it is clearly shown in the
> > > flame graph.
> > >
> > > In fact, radix_tree_lookup() is perfectly fine with RCU read lock,
> > > we don't have to take a spinlock on this hot path. It is pretty
> > > much
> > > similar to commit 291c566a2891
> > > ("net/mlx4_core: Fix racy CQ (Completion Queue) free"). Slow paths
> > > are still serialized with the spinlock, and with synchronize_irq()
> > > it should be safe to just move the fast path to RCU read lock.
> > >
> > > This patch itself reduces the latency by about 50% with our
> > > workload.
> > >
> > > Cc: Saeed Mahameed <saeedm@mellanox.com>
> > > Cc: Tariq Toukan <tariqt@mellanox.com>
> > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> >
> > Acked-by: Saeed Mahameed <saeedm@mellanox.com>
> >
>
> Actually, the commit message needs some rework, since there is no
> contention upstream, Cong can you take care of this and post a V2 ?
I can't verify if upstream has contention or not, but yeah, I can at
least mention the commit 02d92f7903647119e125b24f547 in
changelog.
Thanks.
^ permalink raw reply
* Re: [ISSUE][4.20.6] mlx5 and checksum failures
From: Cong Wang @ 2019-02-06 22:49 UTC (permalink / raw)
To: Ian Kumlien; +Cc: David Miller, Saeed Mahameed, Linux Kernel Network Developers
In-Reply-To: <CAA85sZsbmJ=c69h6yzd0LAzd198=mppjqjPZu_EXhQP+soe4Yw@mail.gmail.com>
On Wed, Feb 6, 2019 at 2:41 PM Ian Kumlien <ian.kumlien@gmail.com> wrote:
>
> On Wed, Feb 6, 2019 at 11:38 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > On Wed, Feb 6, 2019 at 2:15 PM Ian Kumlien <ian.kumlien@gmail.com> wrote:
> > > Could we please schedule this for 4.19 and 4.20 - it's kinda breaking things
> >
> > It doesn't break anything, packets are _not_ dropped, only that the
> > warning itself is noisy.
>
> Not my experience, to me it slows the machine down and looses packets,
> I don't however know
> if this is the only culprit
The packet process could be slow down because of printing
out this kernel warning. Packet should be still delivered to upper
stack, at least I didn't see any packet drops because of this.
>
> You can actually see it on ping where it start out with 0.0xyx and
> ends up at ~10ms
>
I don't understand how it could affect ICMP, it is purely TCP
from my point of view, even the stack trace from you says so. ;)
Thanks.
^ permalink raw reply
* [PATCH net 4/6] qede: Fix system crash on configuring channels.
From: Manish Chopra @ 2019-02-06 22:43 UTC (permalink / raw)
To: davem; +Cc: netdev, aelior, mkalderon, Sudarsana Reddy Kalluru
In-Reply-To: <20190206224347.17054-1-manishc@marvell.com>
From: Sudarsana Reddy Kalluru <skalluru@marvell.com>
Under heavy traffic load, when changing number of channels via
ethtool (ethtool -L) which will cause interface to be reloaded,
it was observed that some packets gets transmitted on old TX
channel/queue id which doesn't really exist after the channel
configuration leads to system crash.
Add a safeguard in the driver by validating queue id through
ndo_select_queue() which is called before the ndo_start_xmit().
Signed-off-by: Sudarsana Reddy Kalluru <skalluru@marvell.com>
Signed-off-by: Ariel Elior <aelior@marvell.com>
---
drivers/net/ethernet/qlogic/qede/qede.h | 3 +++
drivers/net/ethernet/qlogic/qede/qede_fp.c | 13 +++++++++++++
drivers/net/ethernet/qlogic/qede/qede_main.c | 3 +++
3 files changed, 19 insertions(+)
diff --git a/drivers/net/ethernet/qlogic/qede/qede.h b/drivers/net/ethernet/qlogic/qede/qede.h
index 613249d..cd40426 100644
--- a/drivers/net/ethernet/qlogic/qede/qede.h
+++ b/drivers/net/ethernet/qlogic/qede/qede.h
@@ -494,6 +494,9 @@ struct qede_reload_args {
/* Datapath functions definition */
netdev_tx_t qede_start_xmit(struct sk_buff *skb, struct net_device *ndev);
+u16 qede_select_queue(struct net_device *dev, struct sk_buff *skb,
+ struct net_device *sb_dev,
+ select_queue_fallback_t fallback);
netdev_features_t qede_features_check(struct sk_buff *skb,
struct net_device *dev,
netdev_features_t features);
diff --git a/drivers/net/ethernet/qlogic/qede/qede_fp.c b/drivers/net/ethernet/qlogic/qede/qede_fp.c
index bdf816f..31b046e 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_fp.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_fp.c
@@ -1695,6 +1695,19 @@ netdev_tx_t qede_start_xmit(struct sk_buff *skb, struct net_device *ndev)
return NETDEV_TX_OK;
}
+u16 qede_select_queue(struct net_device *dev, struct sk_buff *skb,
+ struct net_device *sb_dev,
+ select_queue_fallback_t fallback)
+{
+ struct qede_dev *edev = netdev_priv(dev);
+ int total_txq;
+
+ total_txq = QEDE_TSS_COUNT(edev) * edev->dev_info.num_tc;
+
+ return QEDE_TSS_COUNT(edev) ?
+ fallback(dev, skb, NULL) % total_txq : 0;
+}
+
/* 8B udp header + 8B base tunnel header + 32B option length */
#define QEDE_MAX_TUN_HDR_LEN 48
diff --git a/drivers/net/ethernet/qlogic/qede/qede_main.c b/drivers/net/ethernet/qlogic/qede/qede_main.c
index 5a74fcb..9790f26 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_main.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_main.c
@@ -631,6 +631,7 @@ static int qede_setup_tc_block(struct qede_dev *edev,
.ndo_open = qede_open,
.ndo_stop = qede_close,
.ndo_start_xmit = qede_start_xmit,
+ .ndo_select_queue = qede_select_queue,
.ndo_set_rx_mode = qede_set_rx_mode,
.ndo_set_mac_address = qede_set_mac_addr,
.ndo_validate_addr = eth_validate_addr,
@@ -666,6 +667,7 @@ static int qede_setup_tc_block(struct qede_dev *edev,
.ndo_open = qede_open,
.ndo_stop = qede_close,
.ndo_start_xmit = qede_start_xmit,
+ .ndo_select_queue = qede_select_queue,
.ndo_set_rx_mode = qede_set_rx_mode,
.ndo_set_mac_address = qede_set_mac_addr,
.ndo_validate_addr = eth_validate_addr,
@@ -684,6 +686,7 @@ static int qede_setup_tc_block(struct qede_dev *edev,
.ndo_open = qede_open,
.ndo_stop = qede_close,
.ndo_start_xmit = qede_start_xmit,
+ .ndo_select_queue = qede_select_queue,
.ndo_set_rx_mode = qede_set_rx_mode,
.ndo_set_mac_address = qede_set_mac_addr,
.ndo_validate_addr = eth_validate_addr,
--
1.8.3.1
^ permalink raw reply related
* [PATCH net 6/6] qed*: Advance drivers version to 8.37.0.20
From: Manish Chopra @ 2019-02-06 22:43 UTC (permalink / raw)
To: davem; +Cc: netdev, aelior, mkalderon
In-Reply-To: <20190206224347.17054-1-manishc@marvell.com>
Version update for qed/qede modules.
Signed-off-by: Manish Chopra <manishc@marvell.com>
Signed-off-by: Ariel Elior <aelior@marvell.com>
---
drivers/net/ethernet/qlogic/qed/qed.h | 2 +-
drivers/net/ethernet/qlogic/qede/qede.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed.h b/drivers/net/ethernet/qlogic/qed/qed.h
index 24a9016..2d8a77c 100644
--- a/drivers/net/ethernet/qlogic/qed/qed.h
+++ b/drivers/net/ethernet/qlogic/qed/qed.h
@@ -53,7 +53,7 @@
extern const struct qed_common_ops qed_common_ops_pass;
#define QED_MAJOR_VERSION 8
-#define QED_MINOR_VERSION 33
+#define QED_MINOR_VERSION 37
#define QED_REVISION_VERSION 0
#define QED_ENGINEERING_VERSION 20
diff --git a/drivers/net/ethernet/qlogic/qede/qede.h b/drivers/net/ethernet/qlogic/qede/qede.h
index cd40426..730997b 100644
--- a/drivers/net/ethernet/qlogic/qede/qede.h
+++ b/drivers/net/ethernet/qlogic/qede/qede.h
@@ -56,7 +56,7 @@
#include <net/tc_act/tc_gact.h>
#define QEDE_MAJOR_VERSION 8
-#define QEDE_MINOR_VERSION 33
+#define QEDE_MINOR_VERSION 37
#define QEDE_REVISION_VERSION 0
#define QEDE_ENGINEERING_VERSION 20
#define DRV_MODULE_VERSION __stringify(QEDE_MAJOR_VERSION) "." \
--
1.8.3.1
^ permalink raw reply related
* [PATCH net 5/6] qed: Change verbosity for coalescing message.
From: Manish Chopra @ 2019-02-06 22:43 UTC (permalink / raw)
To: davem; +Cc: netdev, aelior, mkalderon, Rahul Verma
In-Reply-To: <20190206224347.17054-1-manishc@marvell.com>
From: Rahul Verma <rverma@marvell.com>
Fix unnecessary logging of message in an expected
default case where coalescing value read (via ethtool -c)
migh not be valid unless they are configured explicitly
in the hardware using ethtool -C.
Signed-off-by: Rahul Verma <rverma@marvell.com>
Signed-off-by: Manish Chopra <manishc@marvell.com>
Signed-off-by: Ariel Elior <aelior@marvell.com>
---
drivers/net/ethernet/qlogic/qed/qed_l2.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_l2.c b/drivers/net/ethernet/qlogic/qed/qed_l2.c
index 64ac95c..58be1c4 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_l2.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_l2.c
@@ -2871,7 +2871,8 @@ static int qed_get_coalesce(struct qed_dev *cdev, u16 *coal, void *handle)
p_hwfn = p_cid->p_owner;
rc = qed_get_queue_coalesce(p_hwfn, coal, handle);
if (rc)
- DP_NOTICE(p_hwfn, "Unable to read queue coalescing\n");
+ DP_VERBOSE(cdev, QED_MSG_DEBUG,
+ "Unable to read queue coalescing\n");
return rc;
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH net 3/6] qed: Consider TX tcs while deriving the max num_queues for PF.
From: Manish Chopra @ 2019-02-06 22:43 UTC (permalink / raw)
To: davem; +Cc: netdev, aelior, mkalderon, Sudarsana Reddy Kalluru
In-Reply-To: <20190206224347.17054-1-manishc@marvell.com>
From: Sudarsana Reddy Kalluru <skalluru@marvell.com>
Max supported queues is derived incorrectly in the case of multi-CoS.
Need to consider TCs while calculating num_queues for PF.
Signed-off-by: Sudarsana Reddy Kalluru <skalluru@marvell.com>
Signed-off-by: Ariel Elior <aelior@marvell.com>
---
drivers/net/ethernet/qlogic/qed/qed_l2.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_l2.c b/drivers/net/ethernet/qlogic/qed/qed_l2.c
index e68ca83..64ac95c 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_l2.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_l2.c
@@ -2216,7 +2216,7 @@ static int qed_fill_eth_dev_info(struct qed_dev *cdev,
u16 num_queues = 0;
/* Since the feature controls only queue-zones,
- * make sure we have the contexts [rx, tx, xdp] to
+ * make sure we have the contexts [rx, xdp, tcs] to
* match.
*/
for_each_hwfn(cdev, i) {
@@ -2226,7 +2226,8 @@ static int qed_fill_eth_dev_info(struct qed_dev *cdev,
u16 cids;
cids = hwfn->pf_params.eth_pf_params.num_cons;
- num_queues += min_t(u16, l2_queues, cids / 3);
+ cids /= (2 + info->num_tc);
+ num_queues += min_t(u16, l2_queues, cids);
}
/* queues might theoretically be >256, but interrupts'
--
1.8.3.1
^ permalink raw reply related
* [PATCH net 2/6] qed: Assign UFP TC value to vlan priority in UFP mode.
From: Manish Chopra @ 2019-02-06 22:43 UTC (permalink / raw)
To: davem; +Cc: netdev, aelior, mkalderon, Sudarsana Reddy Kalluru
In-Reply-To: <20190206224347.17054-1-manishc@marvell.com>
From: Sudarsana Reddy Kalluru <skalluru@marvell.com>
In the case of Unified Fabric Port (UFP) mode, switch provides
the traffic class (TC) value to be used for the traffic.
Configure hardware to use this TC value for vlan priority.
Signed-off-by: Sudarsana Reddy Kalluru <skalluru@marvell.com>
Signed-off-by: Ariel Elior <aelior@marvell.com>
---
drivers/net/ethernet/qlogic/qed/qed_sp_commands.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_sp_commands.c b/drivers/net/ethernet/qlogic/qed/qed_sp_commands.c
index 888274f..5a495fd 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_sp_commands.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_sp_commands.c
@@ -604,6 +604,9 @@ int qed_sp_pf_update_stag(struct qed_hwfn *p_hwfn)
p_ent->ramrod.pf_update.update_mf_vlan_flag = true;
p_ent->ramrod.pf_update.mf_vlan = cpu_to_le16(p_hwfn->hw_info.ovlan);
+ if (test_bit(QED_MF_UFP_SPECIFIC, &p_hwfn->cdev->mf_bits))
+ p_ent->ramrod.pf_update.mf_vlan |=
+ cpu_to_le16(((u16)p_hwfn->ufp_info.tc << 13));
return qed_spq_post(p_hwfn, p_ent, NULL);
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH net 1/6] qed: Fix EQ full firmware assert.
From: Manish Chopra @ 2019-02-06 22:43 UTC (permalink / raw)
To: davem; +Cc: netdev, aelior, mkalderon
In-Reply-To: <20190206224347.17054-1-manishc@marvell.com>
When slowpath messages are sent with high rate, the resulting
events can lead to a FW assert in case they are not handled fast
enough (Event Queue Full assert). Attempt to send queued slowpath
messages only after the newly evacuated entries in the EQ ring
are indicated to FW.
Signed-off-by: Manish Chopra <manishc@marvell.com>
Signed-off-by: Ariel Elior <aelior@marvell.com>
---
drivers/net/ethernet/qlogic/qed/qed_sp.h | 1 +
drivers/net/ethernet/qlogic/qed/qed_spq.c | 15 +++++++--------
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_sp.h b/drivers/net/ethernet/qlogic/qed/qed_sp.h
index 4179c90..96ab77a 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_sp.h
+++ b/drivers/net/ethernet/qlogic/qed/qed_sp.h
@@ -382,6 +382,7 @@ int qed_spq_completion(struct qed_hwfn *p_hwfn,
* @param p_hwfn
*/
void qed_consq_free(struct qed_hwfn *p_hwfn);
+int qed_spq_pend_post(struct qed_hwfn *p_hwfn);
/**
* @file
diff --git a/drivers/net/ethernet/qlogic/qed/qed_spq.c b/drivers/net/ethernet/qlogic/qed/qed_spq.c
index eb88bbc..ba64ff9 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_spq.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_spq.c
@@ -397,6 +397,11 @@ int qed_eq_completion(struct qed_hwfn *p_hwfn, void *cookie)
qed_eq_prod_update(p_hwfn, qed_chain_get_prod_idx(p_chain));
+ /* Attempt to post pending requests */
+ spin_lock_bh(&p_hwfn->p_spq->lock);
+ rc = qed_spq_pend_post(p_hwfn);
+ spin_unlock_bh(&p_hwfn->p_spq->lock);
+
return rc;
}
@@ -767,7 +772,7 @@ static int qed_spq_post_list(struct qed_hwfn *p_hwfn,
return 0;
}
-static int qed_spq_pend_post(struct qed_hwfn *p_hwfn)
+int qed_spq_pend_post(struct qed_hwfn *p_hwfn)
{
struct qed_spq *p_spq = p_hwfn->p_spq;
struct qed_spq_entry *p_ent = NULL;
@@ -905,7 +910,6 @@ int qed_spq_completion(struct qed_hwfn *p_hwfn,
struct qed_spq_entry *p_ent = NULL;
struct qed_spq_entry *tmp;
struct qed_spq_entry *found = NULL;
- int rc;
if (!p_hwfn)
return -EINVAL;
@@ -963,12 +967,7 @@ int qed_spq_completion(struct qed_hwfn *p_hwfn,
*/
qed_spq_return_entry(p_hwfn, found);
- /* Attempt to post pending requests */
- spin_lock_bh(&p_spq->lock);
- rc = qed_spq_pend_post(p_hwfn);
- spin_unlock_bh(&p_spq->lock);
-
- return rc;
+ return 0;
}
int qed_consq_alloc(struct qed_hwfn *p_hwfn)
--
1.8.3.1
^ permalink raw reply related
* [PATCH net 0/6] qed*: Bug fixes.
From: Manish Chopra @ 2019-02-06 22:43 UTC (permalink / raw)
To: davem; +Cc: netdev, aelior, mkalderon
Hi David,
This series contains general qed/qede fixes.
Please consider applying this to "net"
Thanks,
Manish
Manish Chopra (2):
qed: Fix EQ full firmware assert.
qed*: Advance drivers version to 8.37.0.20
Rahul Verma (1):
qed: Change verbosity for coalescing message.
Sudarsana Reddy Kalluru (3):
qed: Assign UFP TC value to vlan priority in UFP mode.
qed: Consider TX tcs while deriving the max num_queues for PF.
qede: Fix system crash on configuring channels.
drivers/net/ethernet/qlogic/qed/qed.h | 2 +-
drivers/net/ethernet/qlogic/qed/qed_l2.c | 8 +++++---
drivers/net/ethernet/qlogic/qed/qed_sp.h | 1 +
drivers/net/ethernet/qlogic/qed/qed_sp_commands.c | 3 +++
drivers/net/ethernet/qlogic/qed/qed_spq.c | 15 +++++++--------
drivers/net/ethernet/qlogic/qede/qede.h | 5 ++++-
drivers/net/ethernet/qlogic/qede/qede_fp.c | 13 +++++++++++++
drivers/net/ethernet/qlogic/qede/qede_main.c | 3 +++
8 files changed, 37 insertions(+), 13 deletions(-)
--
1.8.3.1
^ permalink raw reply
* Re: [RFC net-next] net: fixed_phy: Move the DT based link GPIO parsing to of_mdio.c
From: Florian Fainelli @ 2019-02-06 22:41 UTC (permalink / raw)
To: Moritz Fischer, Andrew Lunn
Cc: linux-kernel, linux-usb, netdev, colin.king, linus.walleij,
yuehaibing, mcgrof, frowand.list, robh+dt, UNGLinuxDriver,
woojung.huh, hkallweit1, davem, vivien.didelot, moritz,
alex.williams
In-Reply-To: <20190206223846.GA22321@archbook>
On 2/6/19 2:38 PM, Moritz Fischer wrote:
> Hi Andrew,
>
> thanks for your feedback.
>
> On Wed, Feb 06, 2019 at 10:53:22PM +0100, Andrew Lunn wrote:
>> On Wed, Feb 06, 2019 at 12:51:06PM -0800, Moritz Fischer wrote:
>>> Move the DT based link GPIO parsing to of_mdio and let the places
>>> that register a fixed_phy pass in a GPIO descriptor or NULL.
>>>
>>> This allows fixed_phy on non-DT platforms to have link GPIOs, too.
>>>
>>> Signed-off-by: Moritz Fischer <mdf@kernel.org>
>>> ---
>>> drivers/net/dsa/dsa_loop.c | 2 +-
>>> drivers/net/ethernet/broadcom/bgmac.c | 2 +-
>>> drivers/net/ethernet/broadcom/genet/bcmmii.c | 2 +-
>>> drivers/net/phy/fixed_phy.c | 48 ++------------------
>>> drivers/net/usb/lan78xx.c | 2 +-
>>> drivers/of/of_mdio.c | 15 +++++-
>>> include/linux/phy_fixed.h | 3 +-
>>> 7 files changed, 23 insertions(+), 51 deletions(-)
>>>
>>> diff --git a/drivers/net/dsa/dsa_loop.c b/drivers/net/dsa/dsa_loop.c
>>> index 17482ae09aa5..7f124c620092 100644
>>> --- a/drivers/net/dsa/dsa_loop.c
>>> +++ b/drivers/net/dsa/dsa_loop.c
>>> @@ -343,7 +343,7 @@ static int __init dsa_loop_init(void)
>>> unsigned int i;
>>>
>>> for (i = 0; i < NUM_FIXED_PHYS; i++)
>>> - phydevs[i] = fixed_phy_register(PHY_POLL, &status, NULL);
>>> + phydevs[i] = fixed_phy_register(PHY_POLL, &status, NULL, NULL);
>>>
>>> return mdio_driver_register(&dsa_loop_drv);
>>> }
>>> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
>>> index 4632dd5dbad1..bce644dec5c2 100644
>>> --- a/drivers/net/ethernet/broadcom/bgmac.c
>>> +++ b/drivers/net/ethernet/broadcom/bgmac.c
>>> @@ -1446,7 +1446,7 @@ int bgmac_phy_connect_direct(struct bgmac *bgmac)
>>> struct phy_device *phy_dev;
>>> int err;
>>>
>>> - phy_dev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
>>> + phy_dev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL);
>>> if (!phy_dev || IS_ERR(phy_dev)) {
>>> dev_err(bgmac->dev, "Failed to register fixed PHY device\n");
>>> return -ENODEV;
>>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>> index 51880d83131a..7cbd737aba80 100644
>>> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>> @@ -525,7 +525,7 @@ static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv)
>>> .asym_pause = 0,
>>> };
>>>
>>> - phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
>>> + phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL);
>>> if (!phydev || IS_ERR(phydev)) {
>>> dev_err(kdev, "failed to register fixed PHY device\n");
>>> return -ENODEV;
>>> diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
>>> index d810f914aaa4..845bd7c2065a 100644
>>> --- a/drivers/net/phy/fixed_phy.c
>>> +++ b/drivers/net/phy/fixed_phy.c
>>> @@ -18,6 +18,7 @@
>>> #include <linux/err.h>
>>> #include <linux/slab.h>
>>> #include <linux/of.h>
>>> +#include <linux/of_mdio.h>
>>> #include <linux/gpio/consumer.h>
>>> #include <linux/seqlock.h>
>>> #include <linux/idr.h>
>>> @@ -191,50 +192,12 @@ static void fixed_phy_del(int phy_addr)
>>> }
>>> }
>>>
>>> -#ifdef CONFIG_OF_GPIO
>>> -static struct gpio_desc *fixed_phy_get_gpiod(struct device_node *np)
>>> -{
>>> - struct device_node *fixed_link_node;
>>> - struct gpio_desc *gpiod;
>>> -
>>> - if (!np)
>>> - return NULL;
>>> -
>>> - fixed_link_node = of_get_child_by_name(np, "fixed-link");
>>> - if (!fixed_link_node)
>>> - return NULL;
>>> -
>>> - /*
>>> - * As the fixed link is just a device tree node without any
>>> - * Linux device associated with it, we simply have obtain
>>> - * the GPIO descriptor from the device tree like this.
>>> - */
>>> - gpiod = gpiod_get_from_of_node(fixed_link_node, "link-gpios", 0,
>>> - GPIOD_IN, "mdio");
>>> - of_node_put(fixed_link_node);
>>> - if (IS_ERR(gpiod)) {
>>> - if (PTR_ERR(gpiod) == -EPROBE_DEFER)
>>> - return gpiod;
>>> - pr_err("error getting GPIO for fixed link %pOF, proceed without\n",
>>> - fixed_link_node);
>>> - gpiod = NULL;
>>> - }
>>> -
>>> - return gpiod;
>>> -}
>>> -#else
>>> -static struct gpio_desc *fixed_phy_get_gpiod(struct device_node *np)
>>> -{
>>> - return NULL;
>>> -}
>>> -#endif
>>> -
>>> struct phy_device *fixed_phy_register(unsigned int irq,
>>> struct fixed_phy_status *status,
>>> - struct device_node *np)
>>> + struct device_node *np,
>>> + struct gpio_desc *gpiod)
>>> {
>>> struct fixed_mdio_bus *fmb = &platform_fmb;
>>> - struct gpio_desc *gpiod = NULL;
>>> struct phy_device *phy;
>>> int phy_addr;
>>> int ret;
>>> @@ -242,11 +205,6 @@ struct phy_device *fixed_phy_register(unsigned int irq,
>>> if (!fmb->mii_bus || fmb->mii_bus->state != MDIOBUS_REGISTERED)
>>> return ERR_PTR(-EPROBE_DEFER);
>>>
>>> - /* Check if we have a GPIO associated with this fixed phy */
>>> - gpiod = fixed_phy_get_gpiod(np);
>>> - if (IS_ERR(gpiod))
>>> - return ERR_CAST(gpiod);
>>> -
>>> /* Get the next available PHY address, up to PHY_MAX_ADDR */
>>> phy_addr = ida_simple_get(&phy_fixed_ida, 0, PHY_MAX_ADDR, GFP_KERNEL);
>>> if (phy_addr < 0)
>>> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
>>> index 3d92ea6fcc02..bd88f0aef2fa 100644
>>> --- a/drivers/net/usb/lan78xx.c
>>> +++ b/drivers/net/usb/lan78xx.c
>>> @@ -2051,7 +2051,7 @@ static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
>>> phydev = phy_find_first(dev->mdiobus);
>>> if (!phydev) {
>>> netdev_dbg(dev->net, "PHY Not Found!! Registering Fixed PHY\n");
>>> - phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
>>> + phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL);
>>> if (IS_ERR(phydev)) {
>>> netdev_err(dev->net, "No PHY/fixed_PHY found\n");
>>> return NULL;
>>> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
>>> index de6157357e26..6be2120b5f03 100644
>>> --- a/drivers/of/of_mdio.c
>>> +++ b/drivers/of/of_mdio.c
>>> @@ -20,6 +20,7 @@
>>> #include <linux/of_mdio.h>
>>> #include <linux/of_net.h>
>>> #include <linux/module.h>
>>> +#include <linux/gpio/consumer.h>
>>>
>>> #define DEFAULT_GPIO_RESET_DELAY 10 /* in microseconds */
>>>
>>> @@ -460,6 +461,7 @@ int of_phy_register_fixed_link(struct device_node *np)
>>> {
>>> struct fixed_phy_status status = {};
>>> struct device_node *fixed_link_node;
>>> + struct gpio_desc *gpiod = NULL;
>>> u32 fixed_link_prop[5];
>>> const char *managed;
>>>
>>> @@ -483,7 +485,17 @@ int of_phy_register_fixed_link(struct device_node *np)
>>> status.pause = of_property_read_bool(fixed_link_node, "pause");
>>> status.asym_pause = of_property_read_bool(fixed_link_node,
>>> "asym-pause");
>>> +
>>> + gpiod = gpiod_get_from_of_node(fixed_link_node, "link-gpios", 0,
>>> + GPIOD_IN, "mdio");
>>> of_node_put(fixed_link_node);
>>> + if (IS_ERR(gpiod)) {
>>> + if (PTR_ERR(gpiod) == -EPROBE_DEFER)
>>> + return PTR_ERR(gpiod);
>>> + pr_err("error getting GPIO for fixed link %pOF, proceed without\n",
>>> + fixed_link_node);
>>> + gpiod = NULL;
>>> extern struct phy_device *fixed_phy_register(unsigned int irq,
>>> struct fixed_phy_status *status,
>>> - struct device_node *np);
>>> + struct device_node *np,
>>> + struct gpio_desc *gpiod);
>>
>> Hi Moritz
>>
>> I think it would be better to add a
>>
>> extern struct phy_device *fixed_phy_register_gpiod(unsigned int irq,
>> struct fixed_phy_status *status,
>> struct gpio_desc *gpiod);
>>
>> If you are not using DT, the np is pointless. So lets keep the API
>> simple.
>
> To clarify: Do you agree with moving the parsing to of_mdio, or do you
> want to keep the parsing as it was and just add an additional function.
>
> In the latter case you'd have a
> static struct phy_device *__fixed_phy_register(unsigned int irq,
> struct fixed_phy_status *status,
> struct device_node *np,
> struct gpio_desc *gpiod)
> {
> if (gpiod)
> use that
> else
> old behavior / scan dt?
> }
>
> That we then call from fixed_phy_register_gpiod() with a np of NULL,
> and from fixed_phy_register() with a gpiod of NULL?
If you have existing users in tree, of something like:
foo(a, b, c)
and you want to introduce a "d" argument, it's a good practice to do:
foo(a, b, c) -> __foo(a, b, c, 0)
foo_with_d(a, b, c, d) __foo(a, b, c, d)
and have the body of foo() become __foo() while taking the new argument.
Hope this is clear.
--
Florian
^ permalink raw reply
* Re: [ISSUE][4.20.6] mlx5 and checksum failures
From: Ian Kumlien @ 2019-02-06 22:41 UTC (permalink / raw)
To: Cong Wang; +Cc: David Miller, Saeed Mahameed, Linux Kernel Network Developers
In-Reply-To: <CAM_iQpW+mMya6-mPcSdaZQ2rMqs68goFMH5QieUV3WvH43P34Q@mail.gmail.com>
On Wed, Feb 6, 2019 at 11:38 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Wed, Feb 6, 2019 at 2:15 PM Ian Kumlien <ian.kumlien@gmail.com> wrote:
> > Could we please schedule this for 4.19 and 4.20 - it's kinda breaking things
>
> It doesn't break anything, packets are _not_ dropped, only that the
> warning itself is noisy.
Not my experience, to me it slows the machine down and looses packets,
I don't however know
if this is the only culprit
You can actually see it on ping where it start out with 0.0xyx and
ends up at ~10ms
But as I said, I assume this is the culprit - further investigation
will be done =)
^ permalink raw reply
* Re: [RFC net-next] net: fixed_phy: Move the DT based link GPIO parsing to of_mdio.c
From: Moritz Fischer @ 2019-02-06 22:38 UTC (permalink / raw)
To: Andrew Lunn
Cc: Moritz Fischer, linux-kernel, linux-usb, netdev, colin.king,
linus.walleij, yuehaibing, mcgrof, frowand.list, robh+dt,
UNGLinuxDriver, woojung.huh, hkallweit1, davem, f.fainelli,
vivien.didelot, moritz, alex.williams
In-Reply-To: <20190206215322.GC32483@lunn.ch>
Hi Andrew,
thanks for your feedback.
On Wed, Feb 06, 2019 at 10:53:22PM +0100, Andrew Lunn wrote:
> On Wed, Feb 06, 2019 at 12:51:06PM -0800, Moritz Fischer wrote:
> > Move the DT based link GPIO parsing to of_mdio and let the places
> > that register a fixed_phy pass in a GPIO descriptor or NULL.
> >
> > This allows fixed_phy on non-DT platforms to have link GPIOs, too.
> >
> > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > ---
> > drivers/net/dsa/dsa_loop.c | 2 +-
> > drivers/net/ethernet/broadcom/bgmac.c | 2 +-
> > drivers/net/ethernet/broadcom/genet/bcmmii.c | 2 +-
> > drivers/net/phy/fixed_phy.c | 48 ++------------------
> > drivers/net/usb/lan78xx.c | 2 +-
> > drivers/of/of_mdio.c | 15 +++++-
> > include/linux/phy_fixed.h | 3 +-
> > 7 files changed, 23 insertions(+), 51 deletions(-)
> >
> > diff --git a/drivers/net/dsa/dsa_loop.c b/drivers/net/dsa/dsa_loop.c
> > index 17482ae09aa5..7f124c620092 100644
> > --- a/drivers/net/dsa/dsa_loop.c
> > +++ b/drivers/net/dsa/dsa_loop.c
> > @@ -343,7 +343,7 @@ static int __init dsa_loop_init(void)
> > unsigned int i;
> >
> > for (i = 0; i < NUM_FIXED_PHYS; i++)
> > - phydevs[i] = fixed_phy_register(PHY_POLL, &status, NULL);
> > + phydevs[i] = fixed_phy_register(PHY_POLL, &status, NULL, NULL);
> >
> > return mdio_driver_register(&dsa_loop_drv);
> > }
> > diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> > index 4632dd5dbad1..bce644dec5c2 100644
> > --- a/drivers/net/ethernet/broadcom/bgmac.c
> > +++ b/drivers/net/ethernet/broadcom/bgmac.c
> > @@ -1446,7 +1446,7 @@ int bgmac_phy_connect_direct(struct bgmac *bgmac)
> > struct phy_device *phy_dev;
> > int err;
> >
> > - phy_dev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
> > + phy_dev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL);
> > if (!phy_dev || IS_ERR(phy_dev)) {
> > dev_err(bgmac->dev, "Failed to register fixed PHY device\n");
> > return -ENODEV;
> > diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
> > index 51880d83131a..7cbd737aba80 100644
> > --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
> > +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
> > @@ -525,7 +525,7 @@ static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv)
> > .asym_pause = 0,
> > };
> >
> > - phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
> > + phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL);
> > if (!phydev || IS_ERR(phydev)) {
> > dev_err(kdev, "failed to register fixed PHY device\n");
> > return -ENODEV;
> > diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
> > index d810f914aaa4..845bd7c2065a 100644
> > --- a/drivers/net/phy/fixed_phy.c
> > +++ b/drivers/net/phy/fixed_phy.c
> > @@ -18,6 +18,7 @@
> > #include <linux/err.h>
> > #include <linux/slab.h>
> > #include <linux/of.h>
> > +#include <linux/of_mdio.h>
> > #include <linux/gpio/consumer.h>
> > #include <linux/seqlock.h>
> > #include <linux/idr.h>
> > @@ -191,50 +192,12 @@ static void fixed_phy_del(int phy_addr)
> > }
> > }
> >
> > -#ifdef CONFIG_OF_GPIO
> > -static struct gpio_desc *fixed_phy_get_gpiod(struct device_node *np)
> > -{
> > - struct device_node *fixed_link_node;
> > - struct gpio_desc *gpiod;
> > -
> > - if (!np)
> > - return NULL;
> > -
> > - fixed_link_node = of_get_child_by_name(np, "fixed-link");
> > - if (!fixed_link_node)
> > - return NULL;
> > -
> > - /*
> > - * As the fixed link is just a device tree node without any
> > - * Linux device associated with it, we simply have obtain
> > - * the GPIO descriptor from the device tree like this.
> > - */
> > - gpiod = gpiod_get_from_of_node(fixed_link_node, "link-gpios", 0,
> > - GPIOD_IN, "mdio");
> > - of_node_put(fixed_link_node);
> > - if (IS_ERR(gpiod)) {
> > - if (PTR_ERR(gpiod) == -EPROBE_DEFER)
> > - return gpiod;
> > - pr_err("error getting GPIO for fixed link %pOF, proceed without\n",
> > - fixed_link_node);
> > - gpiod = NULL;
> > - }
> > -
> > - return gpiod;
> > -}
> > -#else
> > -static struct gpio_desc *fixed_phy_get_gpiod(struct device_node *np)
> > -{
> > - return NULL;
> > -}
> > -#endif
> > -
> > struct phy_device *fixed_phy_register(unsigned int irq,
> > struct fixed_phy_status *status,
> > - struct device_node *np)
> > + struct device_node *np,
> > + struct gpio_desc *gpiod)
> > {
> > struct fixed_mdio_bus *fmb = &platform_fmb;
> > - struct gpio_desc *gpiod = NULL;
> > struct phy_device *phy;
> > int phy_addr;
> > int ret;
> > @@ -242,11 +205,6 @@ struct phy_device *fixed_phy_register(unsigned int irq,
> > if (!fmb->mii_bus || fmb->mii_bus->state != MDIOBUS_REGISTERED)
> > return ERR_PTR(-EPROBE_DEFER);
> >
> > - /* Check if we have a GPIO associated with this fixed phy */
> > - gpiod = fixed_phy_get_gpiod(np);
> > - if (IS_ERR(gpiod))
> > - return ERR_CAST(gpiod);
> > -
> > /* Get the next available PHY address, up to PHY_MAX_ADDR */
> > phy_addr = ida_simple_get(&phy_fixed_ida, 0, PHY_MAX_ADDR, GFP_KERNEL);
> > if (phy_addr < 0)
> > diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> > index 3d92ea6fcc02..bd88f0aef2fa 100644
> > --- a/drivers/net/usb/lan78xx.c
> > +++ b/drivers/net/usb/lan78xx.c
> > @@ -2051,7 +2051,7 @@ static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
> > phydev = phy_find_first(dev->mdiobus);
> > if (!phydev) {
> > netdev_dbg(dev->net, "PHY Not Found!! Registering Fixed PHY\n");
> > - phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
> > + phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL);
> > if (IS_ERR(phydev)) {
> > netdev_err(dev->net, "No PHY/fixed_PHY found\n");
> > return NULL;
> > diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> > index de6157357e26..6be2120b5f03 100644
> > --- a/drivers/of/of_mdio.c
> > +++ b/drivers/of/of_mdio.c
> > @@ -20,6 +20,7 @@
> > #include <linux/of_mdio.h>
> > #include <linux/of_net.h>
> > #include <linux/module.h>
> > +#include <linux/gpio/consumer.h>
> >
> > #define DEFAULT_GPIO_RESET_DELAY 10 /* in microseconds */
> >
> > @@ -460,6 +461,7 @@ int of_phy_register_fixed_link(struct device_node *np)
> > {
> > struct fixed_phy_status status = {};
> > struct device_node *fixed_link_node;
> > + struct gpio_desc *gpiod = NULL;
> > u32 fixed_link_prop[5];
> > const char *managed;
> >
> > @@ -483,7 +485,17 @@ int of_phy_register_fixed_link(struct device_node *np)
> > status.pause = of_property_read_bool(fixed_link_node, "pause");
> > status.asym_pause = of_property_read_bool(fixed_link_node,
> > "asym-pause");
> > +
> > + gpiod = gpiod_get_from_of_node(fixed_link_node, "link-gpios", 0,
> > + GPIOD_IN, "mdio");
> > of_node_put(fixed_link_node);
> > + if (IS_ERR(gpiod)) {
> > + if (PTR_ERR(gpiod) == -EPROBE_DEFER)
> > + return PTR_ERR(gpiod);
> > + pr_err("error getting GPIO for fixed link %pOF, proceed without\n",
> > + fixed_link_node);
> > + gpiod = NULL;
> > extern struct phy_device *fixed_phy_register(unsigned int irq,
> > struct fixed_phy_status *status,
> > - struct device_node *np);
> > + struct device_node *np,
> > + struct gpio_desc *gpiod);
>
> Hi Moritz
>
> I think it would be better to add a
>
> extern struct phy_device *fixed_phy_register_gpiod(unsigned int irq,
> struct fixed_phy_status *status,
> struct gpio_desc *gpiod);
>
> If you are not using DT, the np is pointless. So lets keep the API
> simple.
To clarify: Do you agree with moving the parsing to of_mdio, or do you
want to keep the parsing as it was and just add an additional function.
In the latter case you'd have a
static struct phy_device *__fixed_phy_register(unsigned int irq,
struct fixed_phy_status *status,
struct device_node *np,
struct gpio_desc *gpiod)
{
if (gpiod)
use that
else
old behavior / scan dt?
}
That we then call from fixed_phy_register_gpiod() with a np of NULL,
and from fixed_phy_register() with a gpiod of NULL?
Thanks,
Moritz
^ permalink raw reply
* Re: [ISSUE][4.20.6] mlx5 and checksum failures
From: Cong Wang @ 2019-02-06 22:38 UTC (permalink / raw)
To: Ian Kumlien; +Cc: David Miller, Saeed Mahameed, Linux Kernel Network Developers
In-Reply-To: <CAA85sZt0=a2DDBp0fbVH15DC+=+Q+Rb7ig_09xpNM3smDAT-ZA@mail.gmail.com>
On Wed, Feb 6, 2019 at 2:15 PM Ian Kumlien <ian.kumlien@gmail.com> wrote:
>
> Could we please schedule this for 4.19 and 4.20 - it's kinda breaking things
It doesn't break anything, packets are _not_ dropped, only that the
warning itself is noisy.
^ permalink raw reply
* Re: [ISSUE][4.20.6] mlx5 and checksum failures
From: Ian Kumlien @ 2019-02-06 22:33 UTC (permalink / raw)
To: David Miller; +Cc: Saeed Mahameed, Linux Kernel Network Developers
In-Reply-To: <20190206.142237.482526810806559635.davem@davemloft.net>
On Wed, Feb 6, 2019 at 11:22 PM David Miller <davem@davemloft.net> wrote:
>
> From: Ian Kumlien <ian.kumlien@gmail.com>
> Date: Wed, 6 Feb 2019 23:12:53 +0100
>
> > Could we please schedule this for 4.19 and 4.20 - it's kinda breaking things
>
> Its... there:
>
> https://patchwork.ozlabs.org/bundle/davem/stable/?series=&submitter=&state=*&q=&archive=
F... Sorry, yet again...
I thought that I DID look at patch fork but apparently accepted wasn't
listed by default
^ permalink raw reply
* Re: [PATCH v1] net: dsa: qca8k: implement DT-based ports <-> phy translation
From: Florian Fainelli @ 2019-02-06 22:32 UTC (permalink / raw)
To: Christian Lamparter; +Cc: Andrew Lunn, netdev, Vivien Didelot
In-Reply-To: <14f73250-4255-6f4e-336a-9bf289539b75@gmail.com>
On 2/6/19 2:29 PM, Florian Fainelli wrote:
> On 2/6/19 1:57 PM, Christian Lamparter wrote:
>> On Tuesday, February 5, 2019 11:29:36 PM CET Florian Fainelli wrote:
>>> On 2/5/19 2:12 PM, Christian Lamparter wrote:
>>>> On Tuesday, February 5, 2019 10:29:34 PM CET Andrew Lunn wrote:
>>>>>> For now, I added the DT binding update to the patch as well.
>>>>>> But if this is indeed the way to go, it'll get a separate patch.
>>>>>
>>>>> Hi Christian
>>>>>
>>>>> You need to be careful with the DT binding. You need to keep backwards
>>>>> compatible with it. An old DT blob needs to keep working. I don't
>>>>> think this is true with this change.
>>>>
>>>> Do you mean because of the
>>>>
>>>> - switch0@0 {
>>>> + switch@10 {
>>>> compatible = "qca,qca8337";
>>>> #address-cells = <1>;
>>>> #size-cells = <0>;
>>>>
>>>> - reg = <0>;
>>>> + reg = <0x10>;
>>>>
>>>> change?
>>>>
>>>> or because I removed the phy-handles?>
>>>> The reg = <0x10>; will be necessary regardless. Because this
>>>> is really a bug in the existing binding example and if it is
>>>> copied it will prevent the qca8k driver from loading.
>>>> This is due to a resource conflict, because there will be
>>>> already a "phy_port1: phy@0" registered at reg = <0>;
>>>> So this never worked would have worked.
>>>
>>> That part is fine, it is the removal of the phy-handle properties that
>>> is possibly a problem, but in hindsight, I do not believe it will be a
>>> compatibility issue. Lack of "phy-handle" property within the core DSA
>>> layer means: utilize the switch's internal MDIO bus (ds->slave_mii_bus)
>>> instance, which you are not removing, you are just changing how the PHYs
>>> map to port numbers.
>>>
>> Ok, thanks.
>>
>> I think I'm almost ready for v2. I have fully addressed the compatibility
>> issue by forking off the qca8k_switch_ops depending on whenever a phy-handle
>> property on one of the ports was found or not. If there was no phy-handle the
>> driver adds the slave-bus accessors to the ops which tells DSA to allocate
>> the slave bus and allows the phys can be enumerated. If the phy-handles are
>> found the driver will not have the accessors and DSA will not setup a
>> redundant/fake bus and this prevents the second/double/duplicated discovery
>> and enumeration of the same PHYs again.
>
> The logic you have sounds a little too broad since it stops as soon as
> one port is found with a 'phy-handle' property and assumes that the
> parent MDIO bus from which qca8k itself is a child device, is the MDIO
> bus to be used. There are possibly 3 cases:
>
> 1) All ports using internal/build-in PHYs. In that case, you can either
> not specify a 'phy-handle' property and DSA assumes that they are part
> of the switch's internal MDIO bus. You can also specify a 'phy-handle'
> property that references the internal MDIO bus, although then we also
> expect qca8k to register its internal MDIO bus (ala mv88e6xxx)
>
> 2) Some ports using internal PHYs, some using external PHYs. Similar
> situation again, ports may, or may not specify a 'phy-handle' property,
> so without a 'phy-handle' property that means the port connects to an
> internal PHY, with a 'phy-handle' it could connect to either internal
> PHY or external PHY
>
> 3) All ports using external PHYs, in that case, we must have a
> 'phy-handle' for each port to specify where and how they connect to
> their external PHYs.
>
> With respect to your patch, what I would do is register QCA8k's internal
> MDIO bus as a proper mdio bus and use ds->slave_mii_bus as a storage for
> that bus, such that tell the DSA layer: look, here is the internal MDIO
> bus, would you ever find a port that needs to use a PHY in there.
>
> Then you can still scan each enabled port device, and for each of them,
> populate ds->phys_mii_mask, thus telling DSA exacly which ports are
> using an internal PHY because that would be the ports that do not have a
> 'phy-handle' property. Ports that have a 'phy-handle' property.
Forgot to finish my sentence here. This should read:
Ports that have a 'phy-handle' property will either point to the QCA8k's
internal MDIO bus or an external one, but that will be transparently be
handled during PHY device creation.
Thanks!
--
Florian
^ permalink raw reply
* Re: [PATCH v1] net: dsa: qca8k: implement DT-based ports <-> phy translation
From: Florian Fainelli @ 2019-02-06 22:29 UTC (permalink / raw)
To: Christian Lamparter; +Cc: Andrew Lunn, netdev, Vivien Didelot
In-Reply-To: <3897570.9hORRSCDvi@debian64>
On 2/6/19 1:57 PM, Christian Lamparter wrote:
> On Tuesday, February 5, 2019 11:29:36 PM CET Florian Fainelli wrote:
>> On 2/5/19 2:12 PM, Christian Lamparter wrote:
>>> On Tuesday, February 5, 2019 10:29:34 PM CET Andrew Lunn wrote:
>>>>> For now, I added the DT binding update to the patch as well.
>>>>> But if this is indeed the way to go, it'll get a separate patch.
>>>>
>>>> Hi Christian
>>>>
>>>> You need to be careful with the DT binding. You need to keep backwards
>>>> compatible with it. An old DT blob needs to keep working. I don't
>>>> think this is true with this change.
>>>
>>> Do you mean because of the
>>>
>>> - switch0@0 {
>>> + switch@10 {
>>> compatible = "qca,qca8337";
>>> #address-cells = <1>;
>>> #size-cells = <0>;
>>>
>>> - reg = <0>;
>>> + reg = <0x10>;
>>>
>>> change?
>>>
>>> or because I removed the phy-handles?>
>>> The reg = <0x10>; will be necessary regardless. Because this
>>> is really a bug in the existing binding example and if it is
>>> copied it will prevent the qca8k driver from loading.
>>> This is due to a resource conflict, because there will be
>>> already a "phy_port1: phy@0" registered at reg = <0>;
>>> So this never worked would have worked.
>>
>> That part is fine, it is the removal of the phy-handle properties that
>> is possibly a problem, but in hindsight, I do not believe it will be a
>> compatibility issue. Lack of "phy-handle" property within the core DSA
>> layer means: utilize the switch's internal MDIO bus (ds->slave_mii_bus)
>> instance, which you are not removing, you are just changing how the PHYs
>> map to port numbers.
>>
> Ok, thanks.
>
> I think I'm almost ready for v2. I have fully addressed the compatibility
> issue by forking off the qca8k_switch_ops depending on whenever a phy-handle
> property on one of the ports was found or not. If there was no phy-handle the
> driver adds the slave-bus accessors to the ops which tells DSA to allocate
> the slave bus and allows the phys can be enumerated. If the phy-handles are
> found the driver will not have the accessors and DSA will not setup a
> redundant/fake bus and this prevents the second/double/duplicated discovery
> and enumeration of the same PHYs again.
The logic you have sounds a little too broad since it stops as soon as
one port is found with a 'phy-handle' property and assumes that the
parent MDIO bus from which qca8k itself is a child device, is the MDIO
bus to be used. There are possibly 3 cases:
1) All ports using internal/build-in PHYs. In that case, you can either
not specify a 'phy-handle' property and DSA assumes that they are part
of the switch's internal MDIO bus. You can also specify a 'phy-handle'
property that references the internal MDIO bus, although then we also
expect qca8k to register its internal MDIO bus (ala mv88e6xxx)
2) Some ports using internal PHYs, some using external PHYs. Similar
situation again, ports may, or may not specify a 'phy-handle' property,
so without a 'phy-handle' property that means the port connects to an
internal PHY, with a 'phy-handle' it could connect to either internal
PHY or external PHY
3) All ports using external PHYs, in that case, we must have a
'phy-handle' for each port to specify where and how they connect to
their external PHYs.
With respect to your patch, what I would do is register QCA8k's internal
MDIO bus as a proper mdio bus and use ds->slave_mii_bus as a storage for
that bus, such that tell the DSA layer: look, here is the internal MDIO
bus, would you ever find a port that needs to use a PHY in there.
Then you can still scan each enabled port device, and for each of them,
populate ds->phys_mii_mask, thus telling DSA exacly which ports are
using an internal PHY because that would be the ports that do not have a
'phy-handle' property. Ports that have a 'phy-handle' property.
Hope this helps and is clear, if not, I can try to cook a patch for you
to try, though I don't have you hardware.
Tangential, since you are working on qca8k, it would be great to give
this driver some TLC and make sure that:
- bridge w/ and w/o VLAN filtering enabled works
- multicast snooping works etc.
Cheers
>
> Cheers,
> Christian
>
> Attached is a preview:
>
> ---
> commit 96bc70b4d6e290c450b9af728d9ca0f6db877f13
> Author: Christian Lamparter <chunkeey@gmail.com>
> Date: Fri Feb 1 22:54:32 2019 +0100
>
> net: dsa: qca8k: extend slave-bus implementations
>
> This patch implements accessors for the QCA8337 MDIO access
> through the MDIO_MASTER register, which makes it possible to
> access the PHYs on slave-bus through the switch. In cases
> where the switch ports are already mapped via external
> "phy-phandles", the internal mdio-bus is disabled in order to
> prevent a duplicated discovery and enumeration of the same
> PHYs.
>
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> ---
>
> Changes from v2:
> - Make it compatible with existing configurations
>
> Changes from v1:
> - drop DT port <-> phy mapping
> - added register definitions for the MDIO control register
> - implemented new slave-mdio bus accessors
> - DT-binding: fix switch's PSEUDO_PHY address. It's 0x10 not 0.
>
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index a4b6cda38016..2f1b4b0a3507 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -613,19 +613,61 @@ qca8k_adjust_link(struct dsa_switch *ds, int port, struct phy_device *phy)
> }
>
> static int
> -qca8k_phy_read(struct dsa_switch *ds, int phy, int regnum)
> +qca8k_port_to_phy(int port)
> +{
> + if (port < 1 || port > QCA8K_MDIO_MASTER_MAX_PORTS)
> + return -EINVAL;
> +
> + return port - 1;
> +}
> +
> +static int
> +qca8k_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
> {
> struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
> + u32 val, phy;
> +
> + phy = qca8k_port_to_phy(port);
> + if (phy < 0 || (regnum < 0 || regnum >= QCA8K_MDIO_MASTER_MAX_REG))
> + return -EINVAL;
> +
> + val = QCA8K_MDIO_MASTER_BUSY | QCA8K_MDIO_MASTER_EN |
> + QCA8K_MDIO_MASTER_WRITE | QCA8K_MDIO_MASTER_PHY_ADDR(phy) |
> + QCA8K_MDIO_MASTER_REG_ADDR(regnum) |
> + QCA8K_MDIO_MASTER_DATA(data);
>
> - return mdiobus_read(priv->bus, phy, regnum);
> + qca8k_write(priv, QCA8K_MDIO_MASTER_CTRL, val);
> +
> + return qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
> + QCA8K_MDIO_MASTER_BUSY);
> }
>
> +
> static int
> -qca8k_phy_write(struct dsa_switch *ds, int phy, int regnum, u16 val)
> +qca8k_phy_read(struct dsa_switch *ds, int port, int regnum)
> {
> struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
> + u32 val, phy;
> +
> + phy = qca8k_port_to_phy(port);
> + if (phy < 0 || (regnum < 0 || regnum >= QCA8K_MDIO_MASTER_MAX_REG))
> + return 0xffff;
> +
> + val = QCA8K_MDIO_MASTER_BUSY | QCA8K_MDIO_MASTER_EN |
> + QCA8K_MDIO_MASTER_READ | QCA8K_MDIO_MASTER_PHY_ADDR(phy) |
> + QCA8K_MDIO_MASTER_REG_ADDR(regnum);
> +
> + qca8k_write(priv, QCA8K_MDIO_MASTER_CTRL, val);
>
> - return mdiobus_write(priv->bus, phy, regnum, val);
> + if (qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
> + QCA8K_MDIO_MASTER_BUSY)) {
> + return 0xffff;
> + }
> +
> + val = (qca8k_read(priv, QCA8K_MDIO_MASTER_CTRL) &
> + QCA8K_MDIO_MASTER_DATA_MASK);
> +
> + return val;
> }
>
> static void
> @@ -868,8 +910,6 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
> .setup = qca8k_setup,
> .adjust_link = qca8k_adjust_link,
> .get_strings = qca8k_get_strings,
> - .phy_read = qca8k_phy_read,
> - .phy_write = qca8k_phy_write,
> .get_ethtool_stats = qca8k_get_ethtool_stats,
> .get_sset_count = qca8k_get_sset_count,
> .get_mac_eee = qca8k_get_mac_eee,
> @@ -884,6 +924,38 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
> .port_fdb_dump = qca8k_port_fdb_dump,
> };
>
> +/* Special code to detect DeviceTrees that use the phy-handle
> + * to map external PHYs to the ports. Only if no phy-handle is
> + * detected the slave bus accessors are getting enabled.
> + */
> +static int qca8k_detect_slave_bus(struct qca8k_priv *priv)
> +{
> + struct device_node *ports, *port;
> + bool found = false;
> +
> + ports = of_get_child_by_name(priv->dev->of_node, "ports");
> + if (!ports) {
> + dev_err(priv->dev, "no ports child node found.\n");
> + return -EINVAL;
> + }
> +
> + for_each_available_child_of_node(ports, port) {
> + if (of_property_read_bool(port, "phy-handle")) {
> + found = true;
> + break;
> + }
> + }
> +
> + if (found) {
> + dev_info(priv->dev, "uses external mdio-bus.\n");
> + } else {
> + priv->ops.phy_read = qca8k_phy_read;
> + priv->ops.phy_write = qca8k_phy_write;
> + }
> +
> + return 0;
> +}
> +
> static int
> qca8k_sw_probe(struct mdio_device *mdiodev)
> {
> @@ -912,7 +984,12 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
> return -ENOMEM;
>
> priv->ds->priv = priv;
> - priv->ds->ops = &qca8k_switch_ops;
> + priv->ops = qca8k_switch_ops;
> + if (qca8k_detect_slave_bus(priv))
> + return -EINVAL;
> +
> + priv->ds->ops = &priv->ops;
> +
> mutex_init(&priv->reg_mutex);
> dev_set_drvdata(&mdiodev->dev, priv);
>
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index 613fe5c50236..38d06661f0a8 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -48,6 +48,18 @@
> #define QCA8K_MIB_FLUSH BIT(24)
> #define QCA8K_MIB_CPU_KEEP BIT(20)
> #define QCA8K_MIB_BUSY BIT(17)
> +#define QCA8K_MDIO_MASTER_CTRL 0x3c
> +#define QCA8K_MDIO_MASTER_BUSY BIT(31)
> +#define QCA8K_MDIO_MASTER_EN BIT(30)
> +#define QCA8K_MDIO_MASTER_READ BIT(27)
> +#define QCA8K_MDIO_MASTER_WRITE 0
> +#define QCA8K_MDIO_MASTER_SUP_PRE BIT(26)
> +#define QCA8K_MDIO_MASTER_PHY_ADDR(x) ((x) << 21)
> +#define QCA8K_MDIO_MASTER_REG_ADDR(x) ((x) << 16)
> +#define QCA8K_MDIO_MASTER_DATA(x) (x)
> +#define QCA8K_MDIO_MASTER_DATA_MASK GENMASK(15, 0)
> +#define QCA8K_MDIO_MASTER_MAX_PORTS 5
> +#define QCA8K_MDIO_MASTER_MAX_REG 32
> #define QCA8K_GOL_MAC_ADDR0 0x60
> #define QCA8K_GOL_MAC_ADDR1 0x64
> #define QCA8K_REG_PORT_STATUS(_i) (0x07c + (_i) * 4)
> @@ -168,6 +180,7 @@ struct qca8k_priv {
> struct dsa_switch *ds;
> struct mutex reg_mutex;
> struct device *dev;
> + struct dsa_switch_ops ops;
> };
>
> struct qca8k_mib_desc {
>
>
>
>
>
--
Florian
^ permalink raw reply
* Re: [PATCH net-next] net: phy: fixed_phy: Fix fixed_phy not checking GPIO
From: Moritz Fischer @ 2019-02-06 22:28 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Moritz Fischer, netdev, f.fainelli, hkallweit1, davem
In-Reply-To: <20190206215905.GD32483@lunn.ch>
Hi Andrew,
On Wed, Feb 06, 2019 at 10:59:05PM +0100, Andrew Lunn wrote:
> On Wed, Feb 06, 2019 at 10:10:40AM -0800, Moritz Fischer wrote:
> > Fix fixed_phy not checking GPIO if no link_update callback
> > is registered.
> >
> > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > ---
> >
> > Hi all,
> >
> > I've been trying to figure out where exactly this broke,
> > it must've been somewhere when the file was refactored
> > in connection with phylink?
>
> Hi Moritz
>
> With a quick inspection, i also cannot see where it broken.
>
> I think part of the issue is that all the current users have moved
> onto using phylink, and phylink polls the GPIO, rather than having
> fixed_link do it.
Yeah, I suspected at much :) I still feel we should fix fixed_phy as
long as there are still users for it.
>
> I would prefer to understand exactly which change broke it. Without
> knowing how it broke, it is hard to say if this is the correct fix.
It might've been always this way. That being said I don't see why
one should've to implement an empty function (link_update) in my driver
just to read back the GPIO pin. Looking at the code it seems clear that
nothing else polls the GPIO, which doesn't seem right.
In my current understanding (correct me if I'm wrong), the link_update
callback would give the MAC a chance to update link parameters that
since we are a fixed phy we cannot read back from a PHY.
That seems conceptually independent from obtaining a link up/down info
from a GPIO pin. Wouldn't you agree?
>
> What is your use-case? You Cc: the usb list. So a USB-Ethernet dongle?
> But then why fixed-link?
Not for this patch, did I ? I ran into this when testing nixge, but it
seemed unrelated with my changes.
Thanks,
Moritz
^ permalink raw reply
* Re: [ISSUE][4.20.6] mlx5 and checksum failures
From: David Miller @ 2019-02-06 22:22 UTC (permalink / raw)
To: ian.kumlien; +Cc: saeedm, netdev
In-Reply-To: <CAA85sZt0=a2DDBp0fbVH15DC+=+Q+Rb7ig_09xpNM3smDAT-ZA@mail.gmail.com>
From: Ian Kumlien <ian.kumlien@gmail.com>
Date: Wed, 6 Feb 2019 23:12:53 +0100
> Could we please schedule this for 4.19 and 4.20 - it's kinda breaking things
Its... there:
https://patchwork.ozlabs.org/bundle/davem/stable/?series=&submitter=&state=*&q=&archive=
^ permalink raw reply
* Re: [PATCH net-next v4 00/12] net: Introduce ndo_get_port_parent_id()
From: David Miller @ 2019-02-06 22:18 UTC (permalink / raw)
To: f.fainelli
Cc: netdev, idosch, linux-kernel, linux-rdma, oss-drivers, devel,
bridge
In-Reply-To: <20190206.135050.939253959263316807.davem@davemloft.net>
From: David Miller <davem@davemloft.net>
Date: Wed, 06 Feb 2019 13:50:50 -0800 (PST)
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Wed, 6 Feb 2019 09:45:34 -0800
>
>> Based on discussion with Ido and feedback from Jakub there are clearly
>> two classes of users that implement SWITCHDEV_ATTR_ID_PORT_PARENT_ID:
>>
>> - PF/VF drivers which typically only implement return the port's parent
>> ID, yet have to implement switchdev_port_attr_get() just for that
>>
>> - Ethernet switch drivers: mlxsw, ocelot, DSA, etc. which implement more
>> attributes which we want to be able to eventually veto in the context
>> of the caller, thus making them candidates for using a blocking notifier
>> chain
>>
>> Changes in v4:
> ..
>
> Series applied, thanks Florian.
>
> I'll push this out to net-next after my build tests complete.
I had to remove the unused variable 'rocker' to kill a warning introduced
by patch #8.
Just FYI...
^ permalink raw reply
* Re: [ISSUE][4.20.6] mlx5 and checksum failures
From: Ian Kumlien @ 2019-02-06 22:12 UTC (permalink / raw)
To: David Miller; +Cc: Saeed Mahameed, Linux Kernel Network Developers
In-Reply-To: <20190206.140314.1579572356051091256.davem@davemloft.net>
On Wed, Feb 6, 2019 at 11:03 PM David Miller <davem@davemloft.net> wrote:
> From: Saeed Mahameed <saeedm@mellanox.com>
> Date: Wed, 6 Feb 2019 17:18:55 +0000
> > On Wed, 2019-02-06 at 17:16 +0100, Ian Kumlien wrote:
> >> Hi,
> >>
> >> I'm hitting an issue that i think is fixed by the following patch,
> >> i haven't verified it yet - but it looks like it should go on the
> >> stable queue(?)
> >>
> >> (And yes, I did look, and couldn't find it ;))
> >>
> >
> > Yes, i couldn't find it neither,
> >
> > It should have been queued up for 4.18 by now.
> > Dave said he will take care of it, maybe he just forgot or something.
> > since the patch needed some extra care..
> >
> > https://patchwork.ozlabs.org/patch/1027837/
> >
> > Dave, Is there anything i can do here ?
>
> I never handle anything past the most recent two -stable releases,
> which right now is 4.20 and 4.19
Could we please schedule this for 4.19 and 4.20 - it's kinda breaking things
> For anything beyond that you have to contact the person who maintains
> that -stable tree.
I think it needs to be applied to all -stable since 4.18 :/
^ permalink raw reply
* Re: [PATCH net] net: dsa: Fix NULL checking in dsa_slave_set_eee()
From: David Miller @ 2019-02-06 22:03 UTC (permalink / raw)
To: dan.carpenter; +Cc: andrew, vivien.didelot, f.fainelli, netdev, kernel-janitors
In-Reply-To: <20190206153514.GA11033@kadam>
From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Wed, 6 Feb 2019 18:35:15 +0300
> This function can't succeed if dp->pl is NULL. It will Oops inside the
> call to return phylink_ethtool_get_eee(dp->pl, e);
>
> Fixes: 1be52e97ed3e ("dsa: slave: eee: Allow ports to use phylink")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Applied and queued up for -stable.
^ permalink raw reply
* Re: [ISSUE][4.20.6] mlx5 and checksum failures
From: David Miller @ 2019-02-06 22:03 UTC (permalink / raw)
To: saeedm; +Cc: ian.kumlien, netdev
In-Reply-To: <067294dc4721cd52b87ccc579033b59336588bdd.camel@mellanox.com>
From: Saeed Mahameed <saeedm@mellanox.com>
Date: Wed, 6 Feb 2019 17:18:55 +0000
> On Wed, 2019-02-06 at 17:16 +0100, Ian Kumlien wrote:
>> Hi,
>>
>> I'm hitting an issue that i think is fixed by the following patch,
>> i haven't verified it yet - but it looks like it should go on the
>> stable queue(?)
>>
>> (And yes, I did look, and couldn't find it ;))
>>
>
> Yes, i couldn't find it neither,
>
> It should have been queued up for 4.18 by now.
> Dave said he will take care of it, maybe he just forgot or something.
> since the patch needed some extra care..
>
> https://patchwork.ozlabs.org/patch/1027837/
>
> Dave, Is there anything i can do here ?
I never handle anything past the most recent two -stable releases,
which right now is 4.20 and 4.19
For anything beyond that you have to contact the person who maintains
that -stable tree.
^ permalink raw reply
* Re: [PATCH net-next] net: phy: fixed_phy: Fix fixed_phy not checking GPIO
From: Andrew Lunn @ 2019-02-06 21:59 UTC (permalink / raw)
To: Moritz Fischer; +Cc: netdev, f.fainelli, hkallweit1, davem
In-Reply-To: <20190206181040.29539-1-mdf@kernel.org>
On Wed, Feb 06, 2019 at 10:10:40AM -0800, Moritz Fischer wrote:
> Fix fixed_phy not checking GPIO if no link_update callback
> is registered.
>
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
>
> Hi all,
>
> I've been trying to figure out where exactly this broke,
> it must've been somewhere when the file was refactored
> in connection with phylink?
Hi Moritz
With a quick inspection, i also cannot see where it broken.
I think part of the issue is that all the current users have moved
onto using phylink, and phylink polls the GPIO, rather than having
fixed_link do it.
I would prefer to understand exactly which change broke it. Without
knowing how it broke, it is hard to say if this is the correct fix.
What is your use-case? You Cc: the usb list. So a USB-Ethernet dongle?
But then why fixed-link?
Andrew
^ permalink raw reply
* Re: [PATCH v1] net: dsa: qca8k: implement DT-based ports <-> phy translation
From: Christian Lamparter @ 2019-02-06 21:57 UTC (permalink / raw)
To: Florian Fainelli; +Cc: Andrew Lunn, netdev, Vivien Didelot
In-Reply-To: <699dfb8a-11d4-b27a-e762-ff4fdff1a3d7@gmail.com>
On Tuesday, February 5, 2019 11:29:36 PM CET Florian Fainelli wrote:
> On 2/5/19 2:12 PM, Christian Lamparter wrote:
> > On Tuesday, February 5, 2019 10:29:34 PM CET Andrew Lunn wrote:
> >>> For now, I added the DT binding update to the patch as well.
> >>> But if this is indeed the way to go, it'll get a separate patch.
> >>
> >> Hi Christian
> >>
> >> You need to be careful with the DT binding. You need to keep backwards
> >> compatible with it. An old DT blob needs to keep working. I don't
> >> think this is true with this change.
> >
> > Do you mean because of the
> >
> > - switch0@0 {
> > + switch@10 {
> > compatible = "qca,qca8337";
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > - reg = <0>;
> > + reg = <0x10>;
> >
> > change?
> >
> > or because I removed the phy-handles?>
> > The reg = <0x10>; will be necessary regardless. Because this
> > is really a bug in the existing binding example and if it is
> > copied it will prevent the qca8k driver from loading.
> > This is due to a resource conflict, because there will be
> > already a "phy_port1: phy@0" registered at reg = <0>;
> > So this never worked would have worked.
>
> That part is fine, it is the removal of the phy-handle properties that
> is possibly a problem, but in hindsight, I do not believe it will be a
> compatibility issue. Lack of "phy-handle" property within the core DSA
> layer means: utilize the switch's internal MDIO bus (ds->slave_mii_bus)
> instance, which you are not removing, you are just changing how the PHYs
> map to port numbers.
>
Ok, thanks.
I think I'm almost ready for v2. I have fully addressed the compatibility
issue by forking off the qca8k_switch_ops depending on whenever a phy-handle
property on one of the ports was found or not. If there was no phy-handle the
driver adds the slave-bus accessors to the ops which tells DSA to allocate
the slave bus and allows the phys can be enumerated. If the phy-handles are
found the driver will not have the accessors and DSA will not setup a
redundant/fake bus and this prevents the second/double/duplicated discovery
and enumeration of the same PHYs again.
Cheers,
Christian
Attached is a preview:
---
commit 96bc70b4d6e290c450b9af728d9ca0f6db877f13
Author: Christian Lamparter <chunkeey@gmail.com>
Date: Fri Feb 1 22:54:32 2019 +0100
net: dsa: qca8k: extend slave-bus implementations
This patch implements accessors for the QCA8337 MDIO access
through the MDIO_MASTER register, which makes it possible to
access the PHYs on slave-bus through the switch. In cases
where the switch ports are already mapped via external
"phy-phandles", the internal mdio-bus is disabled in order to
prevent a duplicated discovery and enumeration of the same
PHYs.
Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
Changes from v2:
- Make it compatible with existing configurations
Changes from v1:
- drop DT port <-> phy mapping
- added register definitions for the MDIO control register
- implemented new slave-mdio bus accessors
- DT-binding: fix switch's PSEUDO_PHY address. It's 0x10 not 0.
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index a4b6cda38016..2f1b4b0a3507 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -613,19 +613,61 @@ qca8k_adjust_link(struct dsa_switch *ds, int port, struct phy_device *phy)
}
static int
-qca8k_phy_read(struct dsa_switch *ds, int phy, int regnum)
+qca8k_port_to_phy(int port)
+{
+ if (port < 1 || port > QCA8K_MDIO_MASTER_MAX_PORTS)
+ return -EINVAL;
+
+ return port - 1;
+}
+
+static int
+qca8k_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
{
struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
+ u32 val, phy;
+
+ phy = qca8k_port_to_phy(port);
+ if (phy < 0 || (regnum < 0 || regnum >= QCA8K_MDIO_MASTER_MAX_REG))
+ return -EINVAL;
+
+ val = QCA8K_MDIO_MASTER_BUSY | QCA8K_MDIO_MASTER_EN |
+ QCA8K_MDIO_MASTER_WRITE | QCA8K_MDIO_MASTER_PHY_ADDR(phy) |
+ QCA8K_MDIO_MASTER_REG_ADDR(regnum) |
+ QCA8K_MDIO_MASTER_DATA(data);
- return mdiobus_read(priv->bus, phy, regnum);
+ qca8k_write(priv, QCA8K_MDIO_MASTER_CTRL, val);
+
+ return qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
+ QCA8K_MDIO_MASTER_BUSY);
}
+
static int
-qca8k_phy_write(struct dsa_switch *ds, int phy, int regnum, u16 val)
+qca8k_phy_read(struct dsa_switch *ds, int port, int regnum)
{
struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
+ u32 val, phy;
+
+ phy = qca8k_port_to_phy(port);
+ if (phy < 0 || (regnum < 0 || regnum >= QCA8K_MDIO_MASTER_MAX_REG))
+ return 0xffff;
+
+ val = QCA8K_MDIO_MASTER_BUSY | QCA8K_MDIO_MASTER_EN |
+ QCA8K_MDIO_MASTER_READ | QCA8K_MDIO_MASTER_PHY_ADDR(phy) |
+ QCA8K_MDIO_MASTER_REG_ADDR(regnum);
+
+ qca8k_write(priv, QCA8K_MDIO_MASTER_CTRL, val);
- return mdiobus_write(priv->bus, phy, regnum, val);
+ if (qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
+ QCA8K_MDIO_MASTER_BUSY)) {
+ return 0xffff;
+ }
+
+ val = (qca8k_read(priv, QCA8K_MDIO_MASTER_CTRL) &
+ QCA8K_MDIO_MASTER_DATA_MASK);
+
+ return val;
}
static void
@@ -868,8 +910,6 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
.setup = qca8k_setup,
.adjust_link = qca8k_adjust_link,
.get_strings = qca8k_get_strings,
- .phy_read = qca8k_phy_read,
- .phy_write = qca8k_phy_write,
.get_ethtool_stats = qca8k_get_ethtool_stats,
.get_sset_count = qca8k_get_sset_count,
.get_mac_eee = qca8k_get_mac_eee,
@@ -884,6 +924,38 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
.port_fdb_dump = qca8k_port_fdb_dump,
};
+/* Special code to detect DeviceTrees that use the phy-handle
+ * to map external PHYs to the ports. Only if no phy-handle is
+ * detected the slave bus accessors are getting enabled.
+ */
+static int qca8k_detect_slave_bus(struct qca8k_priv *priv)
+{
+ struct device_node *ports, *port;
+ bool found = false;
+
+ ports = of_get_child_by_name(priv->dev->of_node, "ports");
+ if (!ports) {
+ dev_err(priv->dev, "no ports child node found.\n");
+ return -EINVAL;
+ }
+
+ for_each_available_child_of_node(ports, port) {
+ if (of_property_read_bool(port, "phy-handle")) {
+ found = true;
+ break;
+ }
+ }
+
+ if (found) {
+ dev_info(priv->dev, "uses external mdio-bus.\n");
+ } else {
+ priv->ops.phy_read = qca8k_phy_read;
+ priv->ops.phy_write = qca8k_phy_write;
+ }
+
+ return 0;
+}
+
static int
qca8k_sw_probe(struct mdio_device *mdiodev)
{
@@ -912,7 +984,12 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
return -ENOMEM;
priv->ds->priv = priv;
- priv->ds->ops = &qca8k_switch_ops;
+ priv->ops = qca8k_switch_ops;
+ if (qca8k_detect_slave_bus(priv))
+ return -EINVAL;
+
+ priv->ds->ops = &priv->ops;
+
mutex_init(&priv->reg_mutex);
dev_set_drvdata(&mdiodev->dev, priv);
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 613fe5c50236..38d06661f0a8 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -48,6 +48,18 @@
#define QCA8K_MIB_FLUSH BIT(24)
#define QCA8K_MIB_CPU_KEEP BIT(20)
#define QCA8K_MIB_BUSY BIT(17)
+#define QCA8K_MDIO_MASTER_CTRL 0x3c
+#define QCA8K_MDIO_MASTER_BUSY BIT(31)
+#define QCA8K_MDIO_MASTER_EN BIT(30)
+#define QCA8K_MDIO_MASTER_READ BIT(27)
+#define QCA8K_MDIO_MASTER_WRITE 0
+#define QCA8K_MDIO_MASTER_SUP_PRE BIT(26)
+#define QCA8K_MDIO_MASTER_PHY_ADDR(x) ((x) << 21)
+#define QCA8K_MDIO_MASTER_REG_ADDR(x) ((x) << 16)
+#define QCA8K_MDIO_MASTER_DATA(x) (x)
+#define QCA8K_MDIO_MASTER_DATA_MASK GENMASK(15, 0)
+#define QCA8K_MDIO_MASTER_MAX_PORTS 5
+#define QCA8K_MDIO_MASTER_MAX_REG 32
#define QCA8K_GOL_MAC_ADDR0 0x60
#define QCA8K_GOL_MAC_ADDR1 0x64
#define QCA8K_REG_PORT_STATUS(_i) (0x07c + (_i) * 4)
@@ -168,6 +180,7 @@ struct qca8k_priv {
struct dsa_switch *ds;
struct mutex reg_mutex;
struct device *dev;
+ struct dsa_switch_ops ops;
};
struct qca8k_mib_desc {
^ permalink raw reply related
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
From: Julien Gomes @ 2019-02-06 21:53 UTC (permalink / raw)
To: Neil Horman
Cc: Marcelo Ricardo Leitner, netdev, linux-sctp, linux-kernel, davem,
vyasevich, lucien.xin
In-Reply-To: <271b88ca-ae7c-7d07-330a-242e8ba7322d@arista.com>
On 2/6/19 1:48 PM, Julien Gomes wrote:
>
>
> On 2/6/19 1:39 PM, Neil Horman wrote:
>> On Wed, Feb 06, 2019 at 01:26:55PM -0800, Julien Gomes wrote:
>>>
>>>
>>> On 2/6/19 1:07 PM, Marcelo Ricardo Leitner wrote:
>>>> On Wed, Feb 06, 2019 at 12:48:38PM -0800, Julien Gomes wrote:
>>>>>
>>>>>
>>>>> On 2/6/19 12:37 PM, Marcelo Ricardo Leitner wrote:
>>>>>> On Wed, Feb 06, 2019 at 12:14:30PM -0800, Julien Gomes wrote:
>>>>>>> Make sctp_setsockopt_events() able to accept sctp_event_subscribe
>>>>>>> structures longer than the current definitions.
>>>>>>>
>>>>>>> This should prevent unjustified setsockopt() failures due to struct
>>>>>>> sctp_event_subscribe extensions (as in 4.11 and 4.12) when using
>>>>>>> binaries that should be compatible, but were built with later kernel
>>>>>>> uapi headers.
>>>>>>
>>>>>> Not sure if we support backwards compatibility like this?
>>>>>>
>>>>>> My issue with this change is that by doing this, application will have
>>>>>> no clue if the new bits were ignored or not and it may think that an
>>>>>> event is enabled while it is not.
>>>>>>
>>>>>> A workaround would be to do a getsockopt and check the size that was
>>>>>> returned. But then, it might as well use the right struct here in the
>>>>>> first place.
>>>>>>
>>>>>> I'm seeing current implementation as an implicitly versioned argument:
>>>>>> it will always accept setsockopt calls with an old struct (v4.11 or
>>>>>> v4.12), but if the user tries to use v3 on a v1-only system, it will
>>>>>> be rejected. Pretty much like using a newer setsockopt on an old
>>>>>> system.
>>>>>
>>>>> With the current implementation, given sources that say are supposed to
>>>>> run on a 4.9 kernel (no use of any newer field added in 4.11 or 4.12),
>>>>> we can't rebuild the exact same sources on a 4.19 kernel and still run
>>>>> them on 4.9 without messing with structures re-definition.
>>>>
>>>> Maybe what we want(ed) here then is explicit versioning, to have the 3
>>>> definitions available. Then the application is able to use, say struct
>>>> sctp_event_subscribe, and be happy with it, while there is struct
>>>> sctp_event_subscribe_v2 and struct sctp_event_subscribe_v3 there too.
>>>>
>>>> But it's too late for that now because that would break applications
>>>> already using the new fields in sctp_event_subscribe.
>>>
>>> Right.
>>>
>>>>
>>>>>
>>>>> I understand your point, but this still looks like a sort of uapi
>>>>> breakage to me.
>>>>
>>>> Not disagreeing. I really just don't know how supported that is.
>>>> Willing to know so I can pay more attention to this on future changes.
>>>>
>>>> Btw, is this the only occurrence?
>>>
>>> Can't really say, this is one I witnessed, I haven't really looked for
>>> others.
>>>
>>>>
>>>>>
>>>>>
>>>>> I also had another way to work-around this in mind, by copying optlen
>>>>> bytes and checking that any additional field (not included in the
>>>>> "current" kernel structure definition) is not set, returning EINVAL in
>>>>> such case to keep a similar to current behavior.
>>>>> The issue with this is that I didn't find a suitable (ie not totally
>>>>> arbitrary such as "twice the existing structure size") upper limit to
>>>>> optlen.
>>>>
>>>> Seems interesting. Why would it need that upper limit to optlen?
>>>>
>>>> Say struct v1 had 4 bytes, v3 now had 12. The user supplies 12 bytes
>>>> to the kernel that only knows about 4 bytes. It can check that (12-4)
>>>> bytes in the end, make sure no bit is on and use only the first 4.
>>>>
>>>> The fact that it was 12 or 200 shouldn't matter, should it? As long as
>>>> the (200-4) bytes are 0'ed, only the first 4 will be used and it
>>>> should be ok, otherwise EINVAL. No need to know how big the current
>>>> current actually is because it wouldn't be validating that here: just
>>>> that it can safely use the first 4 bytes.
>>>
>>> The upper limit concern is more regarding the call to copy_from_user
>>> with an unrestricted/unchecked value.
>> Copy_from_user should be safe to copy an arbitrary amount, the only restriction
>> is that optlen can't exceed the size of the buffer receiving the data in the
>> kernel. From that standpoint your patch is safe. However, that exposes the
>> problem of checking any tail data on the userspace buffer. That is to say, if
>> you want to ensure that any extra data that gets sent from userspace isn't
>> 'set', you would have to copy that extra data in consumable chunks and check
>> them individaully, and that screams DOS to me (i.e. imagine a user passing in a
>> 4GB buffer, and having to wait for the kernel to copy each X sized chunk,
>> looking for non-zero values).
>
> There probably is a decent compromise to find between "not accepting a
> single additional byte" and accepting several GB.
> For example how likely is it that the growth of this structure make it
> go over a page? I would hope not at all.
>
> By choosing a large but decent high limit, I think we can find a
> future-compatible compromise that doesn't rely on a preliminary
> getsockopt() just for structure trucation decision...
And I was just reminded about huge pages.
But still, my point of finding a compromise still stands.
>
>>
>>> I am not sure of how much of a risk/how exploitable this could be,
>>> that's why I cautiously wanted to limit it in the first place just in case.
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Julien Gomes <julien@arista.com>
>>>>>>> ---
>>>>>>> net/sctp/socket.c | 2 +-
>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>>>>>> index 9644bdc8e85c..f9717e2789da 100644
>>>>>>> --- a/net/sctp/socket.c
>>>>>>> +++ b/net/sctp/socket.c
>>>>>>> @@ -2311,7 +2311,7 @@ static int sctp_setsockopt_events(struct sock *sk, char __user *optval,
>>>>>>> int i;
>>>>>>>
>>>>>>> if (optlen > sizeof(struct sctp_event_subscribe))
>>>>>>> - return -EINVAL;
>>>>>>> + optlen = sizeof(struct sctp_event_subscribe);
>>>>>>>
>>>>>>> if (copy_from_user(&subscribe, optval, optlen))
>>>>>>> return -EFAULT;
>>>>>>> --
>>>>>>> 2.20.1
>>>>>>>
>>>>>
>>>
>>> --
>>> Julien Gomes
>>>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox