This series is purely non functional. It changes the
lan9303_enable_packet_processing,
lan9303_disable_packet_processing() to pass port number (0,1,2) as
parameter instead of port offset. This aligns them with
other functions in the module, and makes it possible to simplify the code.
First patch: Change lan9303_xxx_packet_processing parameter:
- Pass port number (0,1,2) as parameter.
- Introduced lan9303_write_switch_port()
- Plus replaced a constant 0x400 with LAN9303_SWITCH_PORT_REG()
Second patch: Introduce LAN9303_NUM_PORTS=3, used in next patch.
Third patch: Simplify lan9303_xxx_packet_processing usage.
Comments welcome!
Changes v1 -> v2:
- introduced lan9303_write_switch_port() in first patch
- inserted LAN9303_NUM_PORTS patch
- Use LAN9303_NUM_PORTS in last patch. Plus whitespace change.
Egil Hjelmeland (3):
net: dsa: lan9303: Refactor lan9303_xxx_packet_processing()
net: dsa: lan9303: define LAN9303_NUM_PORTS 3
net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage
drivers/net/dsa/lan9303-core.c | 78 ++++++++++++++++++++++--------------------
1 file changed, 40 insertions(+), 38 deletions(-)
--
2.11.0
lan9303_enable_packet_processing, lan9303_disable_packet_processing()
Pass port number (0,1,2) as parameter instead of port offset.
Because other functions in the module pass port numbers.
And to enable simplifications in following patch.
Plus replaced a constant 0x400 with LAN9303_SWITCH_PORT_REG().
Signed-off-by: Egil Hjelmeland <[email protected]>
---
drivers/net/dsa/lan9303-core.c | 62 ++++++++++++++++++++++--------------------
1 file changed, 33 insertions(+), 29 deletions(-)
diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 8e430d1ee297..4c514d3b9f68 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -159,9 +159,7 @@
# define LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT1 (BIT(9) | BIT(8))
# define LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0 (BIT(1) | BIT(0))
-#define LAN9303_PORT_0_OFFSET 0x400
-#define LAN9303_PORT_1_OFFSET 0x800
-#define LAN9303_PORT_2_OFFSET 0xc00
+#define LAN9303_SWITCH_PORT_REG(port, reg0) (0x400 * (port) + (reg0))
/* the built-in PHYs are of type LAN911X */
#define MII_LAN911X_SPECIAL_MODES 0x12
@@ -428,6 +426,13 @@ static int lan9303_read_switch_reg(struct lan9303 *chip, u16 regnum, u32 *val)
return ret;
}
+static int lan9303_write_switch_port(
+ struct lan9303 *chip, unsigned int port, u16 regnum, u32 val)
+{
+ return lan9303_write_switch_reg(
+ chip, LAN9303_SWITCH_PORT_REG(port, regnum), val);
+}
+
static int lan9303_detect_phy_setup(struct lan9303 *chip)
{
int reg;
@@ -458,24 +463,23 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip)
return 0;
}
-#define LAN9303_MAC_RX_CFG_OFFS (LAN9303_MAC_RX_CFG_0 - LAN9303_PORT_0_OFFSET)
-#define LAN9303_MAC_TX_CFG_OFFS (LAN9303_MAC_TX_CFG_0 - LAN9303_PORT_0_OFFSET)
-
static int lan9303_disable_packet_processing(struct lan9303 *chip,
unsigned int port)
{
int ret;
/* disable RX, but keep register reset default values else */
- ret = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_OFFS + port,
- LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES);
+ ret = lan9303_write_switch_port(
+ chip, port, LAN9303_MAC_RX_CFG_0,
+ LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES);
if (ret)
return ret;
/* disable TX, but keep register reset default values else */
- return lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_OFFS + port,
- LAN9303_MAC_TX_CFG_X_TX_IFG_CONFIG_DEFAULT |
- LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE);
+ return lan9303_write_switch_port(
+ chip, port, LAN9303_MAC_TX_CFG_0,
+ LAN9303_MAC_TX_CFG_X_TX_IFG_CONFIG_DEFAULT |
+ LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE);
}
static int lan9303_enable_packet_processing(struct lan9303 *chip,
@@ -484,17 +488,19 @@ static int lan9303_enable_packet_processing(struct lan9303 *chip,
int ret;
/* enable RX and keep register reset default values else */
- ret = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_OFFS + port,
- LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES |
- LAN9303_MAC_RX_CFG_X_RX_ENABLE);
+ ret = lan9303_write_switch_port(
+ chip, port, LAN9303_MAC_RX_CFG_0,
+ LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES |
+ LAN9303_MAC_RX_CFG_X_RX_ENABLE);
if (ret)
return ret;
/* enable TX and keep register reset default values else */
- return lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_OFFS + port,
- LAN9303_MAC_TX_CFG_X_TX_IFG_CONFIG_DEFAULT |
- LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE |
- LAN9303_MAC_TX_CFG_X_TX_ENABLE);
+ return lan9303_write_switch_port(
+ chip, port, LAN9303_MAC_TX_CFG_0,
+ LAN9303_MAC_TX_CFG_X_TX_IFG_CONFIG_DEFAULT |
+ LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE |
+ LAN9303_MAC_TX_CFG_X_TX_ENABLE);
}
/* We want a special working switch:
@@ -558,13 +564,13 @@ static int lan9303_disable_processing(struct lan9303 *chip)
{
int ret;
- ret = lan9303_disable_packet_processing(chip, LAN9303_PORT_0_OFFSET);
+ ret = lan9303_disable_packet_processing(chip, 0);
if (ret)
return ret;
- ret = lan9303_disable_packet_processing(chip, LAN9303_PORT_1_OFFSET);
+ ret = lan9303_disable_packet_processing(chip, 1);
if (ret)
return ret;
- return lan9303_disable_packet_processing(chip, LAN9303_PORT_2_OFFSET);
+ return lan9303_disable_packet_processing(chip, 2);
}
static int lan9303_check_device(struct lan9303 *chip)
@@ -634,7 +640,7 @@ static int lan9303_setup(struct dsa_switch *ds)
if (ret)
dev_err(chip->dev, "failed to separate ports %d\n", ret);
- ret = lan9303_enable_packet_processing(chip, LAN9303_PORT_0_OFFSET);
+ ret = lan9303_enable_packet_processing(chip, 0);
if (ret)
dev_err(chip->dev, "failed to re-enable switching %d\n", ret);
@@ -704,7 +710,7 @@ static void lan9303_get_ethtool_stats(struct dsa_switch *ds, int port,
unsigned int u, poff;
int ret;
- poff = port * 0x400;
+ poff = LAN9303_SWITCH_PORT_REG(port, 0);
for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
ret = lan9303_read_switch_reg(chip,
@@ -757,11 +763,9 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
/* enable internal packet processing */
switch (port) {
case 1:
- return lan9303_enable_packet_processing(chip,
- LAN9303_PORT_1_OFFSET);
+ return lan9303_enable_packet_processing(chip, port);
case 2:
- return lan9303_enable_packet_processing(chip,
- LAN9303_PORT_2_OFFSET);
+ return lan9303_enable_packet_processing(chip, port);
default:
dev_dbg(chip->dev,
"Error: request to power up invalid port %d\n", port);
@@ -778,12 +782,12 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
/* disable internal packet processing */
switch (port) {
case 1:
- lan9303_disable_packet_processing(chip, LAN9303_PORT_1_OFFSET);
+ lan9303_disable_packet_processing(chip, port);
lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1,
MII_BMCR, BMCR_PDOWN);
break;
case 2:
- lan9303_disable_packet_processing(chip, LAN9303_PORT_2_OFFSET);
+ lan9303_disable_packet_processing(chip, port);
lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2,
MII_BMCR, BMCR_PDOWN);
break;
--
2.11.0
Will be used instead of '3' in upcomming patches.
Signed-off-by: Egil Hjelmeland <[email protected]>
---
drivers/net/dsa/lan9303-core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 4c514d3b9f68..2a3c6bf473dd 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -20,6 +20,8 @@
#include "lan9303.h"
+#define LAN9303_NUM_PORTS 3
+
/* 13.2 System Control and Status Registers
* Multiply register number by 4 to get address offset.
*/
--
2.11.0
Simplify usage of lan9303_enable_packet_processing,
lan9303_disable_packet_processing()
Signed-off-by: Egil Hjelmeland <[email protected]>
---
drivers/net/dsa/lan9303-core.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 2a3c6bf473dd..4da580c43751 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -564,15 +564,16 @@ static int lan9303_handle_reset(struct lan9303 *chip)
/* stop processing packets for all ports */
static int lan9303_disable_processing(struct lan9303 *chip)
{
- int ret;
+ int p;
- ret = lan9303_disable_packet_processing(chip, 0);
- if (ret)
- return ret;
- ret = lan9303_disable_packet_processing(chip, 1);
- if (ret)
- return ret;
- return lan9303_disable_packet_processing(chip, 2);
+ for (p = 0; p < LAN9303_NUM_PORTS; p++) {
+ int ret = lan9303_disable_packet_processing(chip, p);
+
+ if (ret)
+ return ret;
+ }
+
+ return 0;
}
static int lan9303_check_device(struct lan9303 *chip)
@@ -765,7 +766,6 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
/* enable internal packet processing */
switch (port) {
case 1:
- return lan9303_enable_packet_processing(chip, port);
case 2:
return lan9303_enable_packet_processing(chip, port);
default:
@@ -784,13 +784,9 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
/* disable internal packet processing */
switch (port) {
case 1:
- lan9303_disable_packet_processing(chip, port);
- lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1,
- MII_BMCR, BMCR_PDOWN);
- break;
case 2:
lan9303_disable_packet_processing(chip, port);
- lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2,
+ lan9303_phy_write(ds, chip->phy_addr_sel_strap + port,
MII_BMCR, BMCR_PDOWN);
break;
default:
--
2.11.0
Hi Egil,
On Tuesday 01 August 2017 13:14:38 Egil Hjelmeland wrote:
> Will be used instead of '3' in upcomming patches.
>
> Signed-off-by: Egil Hjelmeland <[email protected]>
> ---
> drivers/net/dsa/lan9303-core.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/dsa/lan9303-core.c
> b/drivers/net/dsa/lan9303-core.c index 4c514d3b9f68..2a3c6bf473dd 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -20,6 +20,8 @@
>
> #include "lan9303.h"
>
> +#define LAN9303_NUM_PORTS 3
> +
> /* 13.2 System Control and Status Registers
> * Multiply register number by 4 to get address offset.
> */
Maybe we should put this macro into a shared location because
in "net/dsa/tag_lan9303.c" there is already a "#define LAN9303_MAX_PORTS
3".
jb
--
Pengutronix e.K. ? ? ? ? ? ? ? ? ? ? ? ? ?? ?| Juergen Borleis ? ? ? ? ? ? |
Industrial Linux Solutions ? ? ? ? ? ? ? ?? ?| http://www.pengutronix.de/ ?|
On 01. aug. 2017 13:49, Juergen Borleis wrote:
> Hi Egil,
>
> On Tuesday 01 August 2017 13:14:38 Egil Hjelmeland wrote:
>> Will be used instead of '3' in upcomming patches.
>>
>>
>> +#define LAN9303_NUM_PORTS 3
>> +
>
> Maybe we should put this macro into a shared location because
> in "net/dsa/tag_lan9303.c" there is already a "#define LAN9303_MAX_PORTS
> 3".
>
> jb
>
Is there any suitable shared location for such driver specific
definitions?
I could change the name to LAN9303_MAX_PORTS so it the same.
Rhymes better with DSA_MAX_PORTS too.
Let's hear what other mean.
Egil
On Tue, Aug 01, 2017 at 02:31:44PM +0200, Egil Hjelmeland wrote:
> On 01. aug. 2017 13:49, Juergen Borleis wrote:
> >Hi Egil,
> >
> >On Tuesday 01 August 2017 13:14:38 Egil Hjelmeland wrote:
> >>Will be used instead of '3' in upcomming patches.
> >>
> >>
> >>+#define LAN9303_NUM_PORTS 3
> >>+
> >
> >Maybe we should put this macro into a shared location because
> >in "net/dsa/tag_lan9303.c" there is already a "#define LAN9303_MAX_PORTS
> >3".
> >
> >jb
> >
>
> Is there any suitable shared location for such driver specific
> definitions?
> I could change the name to LAN9303_MAX_PORTS so it the same.
> Rhymes better with DSA_MAX_PORTS too.
Hi Egil, Juergen
The other tag drivers do:
if (source_port >= ds->num_ports || !ds->ports[source_port].netdev)
return NULL;
or just
if (!ds->ports[port].netdev)
return NULL;
The first version is the safest, since a malicious switch could return
port 42, and you are accessing way off the end of ds->ports[]. It does
however require you call dsa_switch_alloc() with the correct number of
ports.
Andrew
> @@ -704,7 +710,7 @@ static void lan9303_get_ethtool_stats(struct dsa_switch *ds, int port,
> unsigned int u, poff;
> int ret;
>
> - poff = port * 0x400;
> + poff = LAN9303_SWITCH_PORT_REG(port, 0);
>
> for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
> ret = lan9303_read_switch_reg(chip,
So the actual code is:
for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
ret = lan9303_read_switch_reg(chip,
lan9303_mib[u].offset + poff,
®);
Could this be written as
for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
ret = lan9303_read_switch_port(chip, port, lan9303_mib[u].offset, ®);
It is then clear you are reading the statistics from a port register.
Andrew
On 01. aug. 2017 15:27, Andrew Lunn wrote:
> On Tue, Aug 01, 2017 at 02:31:44PM +0200, Egil Hjelmeland wrote:
>> On 01. aug. 2017 13:49, Juergen Borleis wrote:
>>> Hi Egil,
>>>
>>> On Tuesday 01 August 2017 13:14:38 Egil Hjelmeland wrote:
>>>> Will be used instead of '3' in upcomming patches.
>>>>
>>>>
>>>> +#define LAN9303_NUM_PORTS 3
>>>> +
>>>
>>> Maybe we should put this macro into a shared location because
>>> in "net/dsa/tag_lan9303.c" there is already a "#define LAN9303_MAX_PORTS
>>> 3".
>>>
>>> jb
>>>
>>
>> Is there any suitable shared location for such driver specific
>> definitions?
>> I could change the name to LAN9303_MAX_PORTS so it the same.
>> Rhymes better with DSA_MAX_PORTS too.
>
> Hi Egil, Juergen
>
> The other tag drivers do:
>
> if (source_port >= ds->num_ports || !ds->ports[source_port].netdev)
> return NULL;
>
> or just
>
> if (!ds->ports[port].netdev)
> return NULL;
>
> The first version is the safest, since a malicious switch could return
> port 42, and you are accessing way off the end of ds->ports[]. It does
> however require you call dsa_switch_alloc() with the correct number of
> ports.
>
Sounds like a plan for a later patch, when changing to
dsa_switch_alloc(LAN9303_NUM_PORTS)
> Andrew
>
Egil
On 01. aug. 2017 15:39, Andrew Lunn wrote:
>> @@ -704,7 +710,7 @@ static void lan9303_get_ethtool_stats(struct dsa_switch *ds, int port,
>> unsigned int u, poff;
>> int ret;
>>
>> - poff = port * 0x400;
>> + poff = LAN9303_SWITCH_PORT_REG(port, 0);
>>
>> for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
>> ret = lan9303_read_switch_reg(chip,
>
> So the actual code is:
>
> for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
> ret = lan9303_read_switch_reg(chip,
> lan9303_mib[u].offset + poff,
> ®);
>
> Could this be written as
>
> for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
> ret = lan9303_read_switch_port(chip, port, lan9303_mib[u].offset, ®);
>
> It is then clear you are reading the statistics from a port register.
>
> Andrew
>
Yes it can. Since it is (insignificantly) less efficient, I
chose not to touch it. But I can do it if you like.
Egil
On Tue, Aug 01, 2017 at 03:50:14PM +0200, Egil Hjelmeland wrote:
> On 01. aug. 2017 15:39, Andrew Lunn wrote:
> >>@@ -704,7 +710,7 @@ static void lan9303_get_ethtool_stats(struct dsa_switch *ds, int port,
> >> unsigned int u, poff;
> >> int ret;
> >>- poff = port * 0x400;
> >>+ poff = LAN9303_SWITCH_PORT_REG(port, 0);
> >> for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
> >> ret = lan9303_read_switch_reg(chip,
> >
> >So the actual code is:
> >
> > for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
> > ret = lan9303_read_switch_reg(chip,
> > lan9303_mib[u].offset + poff,
> > ®);
> >
> >Could this be written as
> >
> > for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
> > ret = lan9303_read_switch_port(chip, port, lan9303_mib[u].offset, ®);
> >
> >It is then clear you are reading the statistics from a port register.
> >
> > Andrew
> >
>
> Yes it can. Since it is (insignificantly) less efficient, I
> chose not to touch it. But I can do it if you like.
I doubt it is less efficient. The compiler has seen
lan9303_read_switch_port() and will probably inline it. So what the
optimiser gets to see is probably the same in both cases.
Try generating the assembler listing in both cases, and compare them
make drivers/net/dsa/lan9303-core.lst
Andrew
On 01. aug. 2017 16:02, Andrew Lunn wrote:
> On Tue, Aug 01, 2017 at 03:50:14PM +0200, Egil Hjelmeland wrote:
>> On 01. aug. 2017 15:39, Andrew Lunn wrote:
>>>> @@ -704,7 +710,7 @@ static void lan9303_get_ethtool_stats(struct dsa_switch *ds, int port,
>>>> unsigned int u, poff;
>>>> int ret;
>>>> - poff = port * 0x400;
>>>> + poff = LAN9303_SWITCH_PORT_REG(port, 0);
>>>> for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
>>>> ret = lan9303_read_switch_reg(chip,
>>>
>>> So the actual code is:
>>>
>>> for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
>>> ret = lan9303_read_switch_reg(chip,
>>> lan9303_mib[u].offset + poff,
>>> ®);
>>>
>>> Could this be written as
>>>
>>> for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
>>> ret = lan9303_read_switch_port(chip, port, lan9303_mib[u].offset, ®);
>>>
>>> It is then clear you are reading the statistics from a port register.
>>>
>>> Andrew
>>>
>>
>> Yes it can. Since it is (insignificantly) less efficient, I
>> chose not to touch it. But I can do it if you like.
>
> I doubt it is less efficient. The compiler has seen
> lan9303_read_switch_port() and will probably inline it. So what the
> optimiser gets to see is probably the same in both cases.
>
> Try generating the assembler listing in both cases, and compare them
>
> make drivers/net/dsa/lan9303-core.lst
>
> Andrew
>
Thanks for the tips about generating assembler listing, can be useful
another time. But in this case I trust you :-)
And in this case it does not really matter, because its not in the
data path.
I did try to look at the listing. But I did not quite understand it.
Looks like it is doing both inlining and unrolling.
Anyway, you just decide how you like to have it in this series.
Egil
On 01. aug. 2017 15:27, Andrew Lunn wrote:
> On Tue, Aug 01, 2017 at 02:31:44PM +0200, Egil Hjelmeland wrote:
>> On 01. aug. 2017 13:49, Juergen Borleis wrote:
>>> Hi Egil,
>>>
>>> On Tuesday 01 August 2017 13:14:38 Egil Hjelmeland wrote:
>>>> Will be used instead of '3' in upcomming patches.
>>>>
>>>>
>>>> +#define LAN9303_NUM_PORTS 3
>>>> +
>>>
>>> Maybe we should put this macro into a shared location because
>>> in "net/dsa/tag_lan9303.c" there is already a "#define LAN9303_MAX_PORTS
>>> 3".
>>>
>>> jb
>>>
>>
>> Is there any suitable shared location for such driver specific
>> definitions?
>> I could change the name to LAN9303_MAX_PORTS so it the same.
>> Rhymes better with DSA_MAX_PORTS too.
>
> Hi Egil, Juergen
>
> The other tag drivers do:
>
> if (source_port >= ds->num_ports || !ds->ports[source_port].netdev)
> return NULL;
>
> or just
>
> if (!ds->ports[port].netdev)
> return NULL;
>
> The first version is the safest, since a malicious switch could return
> port 42, and you are accessing way off the end of ds->ports[]. It does
> however require you call dsa_switch_alloc() with the correct number of
> ports.
>
> Andrew
>
Related question: If the driver does dsa_switch_alloc(3), can it then
trust that all "port" params passed in DSA methods will be between
0 and 2?
Egil
> Related question: If the driver does dsa_switch_alloc(3), can it then
> trust that all "port" params passed in DSA methods will be between
> 0 and 2?
Yes.
Andrew