The header and the data of the skb for the inband mgmt requires
to be in little-endian. This is problematic for big-endian system
as the mgmt header is written in the cpu byte order.
Fix this by converting each value for the mgmt header and data to
little-endian, and convert to cpu byte order the mgmt header and
data sent by the switch.
Fixes: 5950c7c0a68c ("net: dsa: qca8k: add support for mgmt read/write in Ethernet packet")
Tested-by: Pawel Dembicki <[email protected]>
Tested-by: Lech Perczak <[email protected]>
Signed-off-by: Christian Marangi <[email protected]>
Reviewed-by: Lech Perczak <[email protected]>
---
drivers/net/dsa/qca/qca8k-8xxx.c | 63 ++++++++++++++++++++++++--------
include/linux/dsa/tag_qca.h | 6 +--
2 files changed, 50 insertions(+), 19 deletions(-)
diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 5669c92c93f7..4bb9b7eac68b 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -137,27 +137,42 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
struct qca8k_mgmt_eth_data *mgmt_eth_data;
struct qca8k_priv *priv = ds->priv;
struct qca_mgmt_ethhdr *mgmt_ethhdr;
+ u32 command;
u8 len, cmd;
+ int i;
mgmt_ethhdr = (struct qca_mgmt_ethhdr *)skb_mac_header(skb);
mgmt_eth_data = &priv->mgmt_eth_data;
- cmd = FIELD_GET(QCA_HDR_MGMT_CMD, mgmt_ethhdr->command);
- len = FIELD_GET(QCA_HDR_MGMT_LENGTH, mgmt_ethhdr->command);
+ command = le32_to_cpu(mgmt_ethhdr->command);
+ cmd = FIELD_GET(QCA_HDR_MGMT_CMD, command);
+ len = FIELD_GET(QCA_HDR_MGMT_LENGTH, command);
/* Make sure the seq match the requested packet */
- if (mgmt_ethhdr->seq == mgmt_eth_data->seq)
+ if (le32_to_cpu(mgmt_ethhdr->seq) == mgmt_eth_data->seq)
mgmt_eth_data->ack = true;
if (cmd == MDIO_READ) {
- mgmt_eth_data->data[0] = mgmt_ethhdr->mdio_data;
+ u32 *val = mgmt_eth_data->data;
+
+ *val = le32_to_cpu(mgmt_ethhdr->mdio_data);
/* Get the rest of the 12 byte of data.
* The read/write function will extract the requested data.
*/
- if (len > QCA_HDR_MGMT_DATA1_LEN)
- memcpy(mgmt_eth_data->data + 1, skb->data,
- QCA_HDR_MGMT_DATA2_LEN);
+ if (len > QCA_HDR_MGMT_DATA1_LEN) {
+ __le32 *data2 = (__le32 *)skb->data;
+ int data_len = min_t(int, QCA_HDR_MGMT_DATA2_LEN,
+ len - QCA_HDR_MGMT_DATA1_LEN);
+
+ val++;
+
+ for (i = sizeof(u32); i <= data_len; i += sizeof(u32)) {
+ *val = le32_to_cpu(*data2);
+ val++;
+ data2++;
+ }
+ }
}
complete(&mgmt_eth_data->rw_done);
@@ -169,8 +184,10 @@ static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *
struct qca_mgmt_ethhdr *mgmt_ethhdr;
unsigned int real_len;
struct sk_buff *skb;
- u32 *data2;
+ __le32 *data2;
+ u32 command;
u16 hdr;
+ int i;
skb = dev_alloc_skb(QCA_HDR_MGMT_PKT_LEN);
if (!skb)
@@ -199,20 +216,32 @@ static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *
hdr |= FIELD_PREP(QCA_HDR_XMIT_DP_BIT, BIT(0));
hdr |= FIELD_PREP(QCA_HDR_XMIT_CONTROL, QCA_HDR_XMIT_TYPE_RW_REG);
- mgmt_ethhdr->command = FIELD_PREP(QCA_HDR_MGMT_ADDR, reg);
- mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_LENGTH, real_len);
- mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_CMD, cmd);
- mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_CHECK_CODE,
+ command = FIELD_PREP(QCA_HDR_MGMT_ADDR, reg);
+ command |= FIELD_PREP(QCA_HDR_MGMT_LENGTH, real_len);
+ command |= FIELD_PREP(QCA_HDR_MGMT_CMD, cmd);
+ command |= FIELD_PREP(QCA_HDR_MGMT_CHECK_CODE,
QCA_HDR_MGMT_CHECK_CODE_VAL);
+ mgmt_ethhdr->command = cpu_to_le32(command);
+
if (cmd == MDIO_WRITE)
- mgmt_ethhdr->mdio_data = *val;
+ mgmt_ethhdr->mdio_data = cpu_to_le32(*val);
mgmt_ethhdr->hdr = htons(hdr);
data2 = skb_put_zero(skb, QCA_HDR_MGMT_DATA2_LEN + QCA_HDR_MGMT_PADDING_LEN);
- if (cmd == MDIO_WRITE && len > QCA_HDR_MGMT_DATA1_LEN)
- memcpy(data2, val + 1, len - QCA_HDR_MGMT_DATA1_LEN);
+ if (cmd == MDIO_WRITE && len > QCA_HDR_MGMT_DATA1_LEN) {
+ int data_len = min_t(int, QCA_HDR_MGMT_DATA2_LEN,
+ len - QCA_HDR_MGMT_DATA1_LEN);
+
+ val++;
+
+ for (i = sizeof(u32); i <= data_len; i += sizeof(u32)) {
+ *data2 = cpu_to_le32(*val);
+ data2++;
+ val++;
+ }
+ }
return skb;
}
@@ -220,9 +249,11 @@ static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *
static void qca8k_mdio_header_fill_seq_num(struct sk_buff *skb, u32 seq_num)
{
struct qca_mgmt_ethhdr *mgmt_ethhdr;
+ u32 seq;
+ seq = FIELD_PREP(QCA_HDR_MGMT_SEQ_NUM, seq_num);
mgmt_ethhdr = (struct qca_mgmt_ethhdr *)skb->data;
- mgmt_ethhdr->seq = FIELD_PREP(QCA_HDR_MGMT_SEQ_NUM, seq_num);
+ mgmt_ethhdr->seq = cpu_to_le32(seq);
}
static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
diff --git a/include/linux/dsa/tag_qca.h b/include/linux/dsa/tag_qca.h
index 50be7cbd93a5..0e176da1e43f 100644
--- a/include/linux/dsa/tag_qca.h
+++ b/include/linux/dsa/tag_qca.h
@@ -61,9 +61,9 @@ struct sk_buff;
/* Special struct emulating a Ethernet header */
struct qca_mgmt_ethhdr {
- u32 command; /* command bit 31:0 */
- u32 seq; /* seq 63:32 */
- u32 mdio_data; /* first 4byte mdio */
+ __le32 command; /* command bit 31:0 */
+ __le32 seq; /* seq 63:32 */
+ __le32 mdio_data; /* first 4byte mdio */
__be16 hdr; /* qca hdr */
} __packed;
--
2.37.2
The switch sends autocast mib in little-endian. This is problematic for
big-endian system as the values needs to be converted.
Fix this by converting each mib value to cpu byte order.
Fixes: 5c957c7ca78c ("net: dsa: qca8k: add support for mib autocast in Ethernet packet")
Tested-by: Pawel Dembicki <[email protected]>
Tested-by: Lech Perczak <[email protected]>
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/net/dsa/qca/qca8k-8xxx.c | 20 ++++++++------------
include/linux/dsa/tag_qca.h | 2 +-
2 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 4bb9b7eac68b..e3e89ce479c6 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -1518,9 +1518,9 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
struct qca8k_priv *priv = ds->priv;
const struct qca8k_mib_desc *mib;
struct mib_ethhdr *mib_ethhdr;
- int i, mib_len, offset = 0;
- u64 *data;
+ __le32 *data2;
u8 port;
+ int i;
mib_ethhdr = (struct mib_ethhdr *)skb_mac_header(skb);
mib_eth_data = &priv->mib_eth_data;
@@ -1532,28 +1532,24 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
if (port != mib_eth_data->req_port)
goto exit;
- data = mib_eth_data->data;
+ data2 = (__le32 *)skb->data;
for (i = 0; i < priv->info->mib_count; i++) {
mib = &ar8327_mib[i];
/* First 3 mib are present in the skb head */
if (i < 3) {
- data[i] = mib_ethhdr->data[i];
+ mib_eth_data->data[i] = le32_to_cpu(mib_ethhdr->data[i]);
continue;
}
- mib_len = sizeof(uint32_t);
-
/* Some mib are 64 bit wide */
if (mib->size == 2)
- mib_len = sizeof(uint64_t);
-
- /* Copy the mib value from packet to the */
- memcpy(data + i, skb->data + offset, mib_len);
+ mib_eth_data->data[i] = le64_to_cpu(*(__le64 *)data2);
+ else
+ mib_eth_data->data[i] = le32_to_cpu(*data2);
- /* Set the offset for the next mib */
- offset += mib_len;
+ data2 += mib->size;
}
exit:
diff --git a/include/linux/dsa/tag_qca.h b/include/linux/dsa/tag_qca.h
index 0e176da1e43f..b1b5720d89a5 100644
--- a/include/linux/dsa/tag_qca.h
+++ b/include/linux/dsa/tag_qca.h
@@ -73,7 +73,7 @@ enum mdio_cmd {
};
struct mib_ethhdr {
- u32 data[3]; /* first 3 mib counter */
+ __le32 data[3]; /* first 3 mib counter */
__be16 hdr; /* qca hdr */
} __packed;
--
2.37.2
> /* Special struct emulating a Ethernet header */
> struct qca_mgmt_ethhdr {
> - u32 command; /* command bit 31:0 */
> - u32 seq; /* seq 63:32 */
> - u32 mdio_data; /* first 4byte mdio */
> + __le32 command; /* command bit 31:0 */
> + __le32 seq; /* seq 63:32 */
> + __le32 mdio_data; /* first 4byte mdio */
> __be16 hdr; /* qca hdr */
> } __packed;
It looks odd that hdr is BE while the rest are LE. Did you check this?
Andrew
On Mon, Oct 10, 2022 at 08:53:01PM +0200, Andrew Lunn wrote:
> > /* Special struct emulating a Ethernet header */
> > struct qca_mgmt_ethhdr {
> > - u32 command; /* command bit 31:0 */
> > - u32 seq; /* seq 63:32 */
> > - u32 mdio_data; /* first 4byte mdio */
> > + __le32 command; /* command bit 31:0 */
> > + __le32 seq; /* seq 63:32 */
> > + __le32 mdio_data; /* first 4byte mdio */
> > __be16 hdr; /* qca hdr */
> > } __packed;
>
> It looks odd that hdr is BE while the rest are LE. Did you check this?
>
> Andrew
Yes we did many test to analyze this and I just checked with some
tcpdump that the hdr is BE everytime. If you want I can provide you some
tcpdump from 2 different systems.
Anyway it looks like this family switch treats the hdr in a standard way
with the network byte order and for anything else stick to LE.
Also as a side note the tagger worked correctly before the mgmt feature
on BE systems and also works correctly now... just any command is slow
as the mgmt system has to timeout and fallback to legacy mdio.
--
Ansuel
On Mon, Oct 10, 2022 at 02:44:46PM +0200, Christian Marangi wrote:
> On Mon, Oct 10, 2022 at 08:53:01PM +0200, Andrew Lunn wrote:
> > > /* Special struct emulating a Ethernet header */
> > > struct qca_mgmt_ethhdr {
> > > - u32 command; /* command bit 31:0 */
> > > - u32 seq; /* seq 63:32 */
> > > - u32 mdio_data; /* first 4byte mdio */
> > > + __le32 command; /* command bit 31:0 */
> > > + __le32 seq; /* seq 63:32 */
> > > + __le32 mdio_data; /* first 4byte mdio */
> > > __be16 hdr; /* qca hdr */
> > > } __packed;
> >
> > It looks odd that hdr is BE while the rest are LE. Did you check this?
> >
> > Andrew
>
> Yes we did many test to analyze this and I just checked with some
> tcpdump that the hdr is BE everytime. If you want I can provide you some
> tcpdump from 2 different systems.
>
> Anyway it looks like this family switch treats the hdr in a standard way
> with the network byte order and for anything else stick to LE.
>
> Also as a side note the tagger worked correctly before the mgmt feature
> on BE systems and also works correctly now... just any command is slow
> as the mgmt system has to timeout and fallback to legacy mdio.
Could you provide a tcpdump?
On Mon, Oct 10, 2022 at 01:14:58PM +0200, Christian Marangi wrote:
> The header and the data of the skb for the inband mgmt requires
> to be in little-endian. This is problematic for big-endian system
> as the mgmt header is written in the cpu byte order.
>
> Fix this by converting each value for the mgmt header and data to
> little-endian, and convert to cpu byte order the mgmt header and
> data sent by the switch.
By any chance, is the endianness of the data configurable?
On Wed, Oct 12, 2022 at 10:24:11AM +0300, Vladimir Oltean wrote:
> On Mon, Oct 10, 2022 at 02:44:46PM +0200, Christian Marangi wrote:
> > On Mon, Oct 10, 2022 at 08:53:01PM +0200, Andrew Lunn wrote:
> > > > /* Special struct emulating a Ethernet header */
> > > > struct qca_mgmt_ethhdr {
> > > > - u32 command; /* command bit 31:0 */
> > > > - u32 seq; /* seq 63:32 */
> > > > - u32 mdio_data; /* first 4byte mdio */
> > > > + __le32 command; /* command bit 31:0 */
> > > > + __le32 seq; /* seq 63:32 */
> > > > + __le32 mdio_data; /* first 4byte mdio */
> > > > __be16 hdr; /* qca hdr */
> > > > } __packed;
> > >
> > > It looks odd that hdr is BE while the rest are LE. Did you check this?
> > >
> > > Andrew
> >
> > Yes we did many test to analyze this and I just checked with some
> > tcpdump that the hdr is BE everytime. If you want I can provide you some
> > tcpdump from 2 different systems.
> >
> > Anyway it looks like this family switch treats the hdr in a standard way
> > with the network byte order and for anything else stick to LE.
> >
> > Also as a side note the tagger worked correctly before the mgmt feature
> > on BE systems and also works correctly now... just any command is slow
> > as the mgmt system has to timeout and fallback to legacy mdio.
>
> Could you provide a tcpdump?
Hi, this [0] is the zip with all the tcpdump.
The main packet to check are the one that are 60 in lenght and > 170 in
length for the autocast mib. I added both LE and BE and for BE I added
the broken and the correct one.
As you notice without following this endianess madness, the switch
doesn't answer to any request.
Hope the dump are not too bloated to understand this problem.
[0] https://we.tl/t-ZpXVObTIh0
--
Ansuel
On Mon, Oct 10, 2022 at 02:44:46PM +0200, Christian Marangi wrote:
> On Mon, Oct 10, 2022 at 08:53:01PM +0200, Andrew Lunn wrote:
> > > /* Special struct emulating a Ethernet header */
> > > struct qca_mgmt_ethhdr {
> > > - u32 command; /* command bit 31:0 */
> > > - u32 seq; /* seq 63:32 */
> > > - u32 mdio_data; /* first 4byte mdio */
> > > + __le32 command; /* command bit 31:0 */
> > > + __le32 seq; /* seq 63:32 */
> > > + __le32 mdio_data; /* first 4byte mdio */
> > > __be16 hdr; /* qca hdr */
> > > } __packed;
> >
> > It looks odd that hdr is BE while the rest are LE. Did you check this?
> >
> > Andrew
>
> Yes we did many test to analyze this and I just checked with some
> tcpdump that the hdr is BE everytime.
That might actual make sense. The comment says:
> > > /* Special struct emulating a Ethernet header */
And hdr is where the Ether type would be, which is network endian,
i.e. big endian.
Andrew
On Wed, Oct 12, 2022 at 10:27:17AM +0300, Vladimir Oltean wrote:
> On Mon, Oct 10, 2022 at 01:14:58PM +0200, Christian Marangi wrote:
> > The header and the data of the skb for the inband mgmt requires
> > to be in little-endian. This is problematic for big-endian system
> > as the mgmt header is written in the cpu byte order.
> >
> > Fix this by converting each value for the mgmt header and data to
> > little-endian, and convert to cpu byte order the mgmt header and
> > data sent by the switch.
>
> By any chance, is the endianness of the data configurable?
By checking the documentation and the regs relevant to global switch
settings it doesn't look like the switch supports any kind of option
about endianess...
--
Ansuel
On Mon, Oct 10, 2022 at 01:14:58PM +0200, Christian Marangi wrote:
> The header and the data of the skb for the inband mgmt requires
> to be in little-endian. This is problematic for big-endian system
> as the mgmt header is written in the cpu byte order.
>
> Fix this by converting each value for the mgmt header and data to
> little-endian, and convert to cpu byte order the mgmt header and
> data sent by the switch.
>
> Fixes: 5950c7c0a68c ("net: dsa: qca8k: add support for mgmt read/write in Ethernet packet")
> Tested-by: Pawel Dembicki <[email protected]>
> Tested-by: Lech Perczak <[email protected]>
> Signed-off-by: Christian Marangi <[email protected]>
> Reviewed-by: Lech Perczak <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
> struct qca8k_priv *priv = ds->priv;
> const struct qca8k_mib_desc *mib;
> struct mib_ethhdr *mib_ethhdr;
> - int i, mib_len, offset = 0;
> - u64 *data;
> + __le32 *data2;
> u8 port;
> + int i;
>
> mib_ethhdr = (struct mib_ethhdr *)skb_mac_header(skb);
> mib_eth_data = &priv->mib_eth_data;
> @@ -1532,28 +1532,24 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
> if (port != mib_eth_data->req_port)
> goto exit;
>
> - data = mib_eth_data->data;
> + data2 = (__le32 *)skb->data;
>
> for (i = 0; i < priv->info->mib_count; i++) {
> mib = &ar8327_mib[i];
>
> /* First 3 mib are present in the skb head */
> if (i < 3) {
> - data[i] = mib_ethhdr->data[i];
> + mib_eth_data->data[i] = le32_to_cpu(mib_ethhdr->data[i]);
> continue;
> }
>
> - mib_len = sizeof(uint32_t);
> -
> /* Some mib are 64 bit wide */
> if (mib->size == 2)
> - mib_len = sizeof(uint64_t);
> -
> - /* Copy the mib value from packet to the */
> - memcpy(data + i, skb->data + offset, mib_len);
> + mib_eth_data->data[i] = le64_to_cpu(*(__le64 *)data2);
> + else
> + mib_eth_data->data[i] = le32_to_cpu(*data2);
Are there any alignment guarantees? The old memcpy did not care if the
source has oddly alignment. But when you start dereferencing a pointed,
you need to be sure those pointers are aligned. You might want to use
get_unaligned_le32 and get_unaligned_le64.
Andrew
On Wed, Oct 12, 2022 at 02:29:26PM +0200, Andrew Lunn wrote:
> On Mon, Oct 10, 2022 at 02:44:46PM +0200, Christian Marangi wrote:
> > On Mon, Oct 10, 2022 at 08:53:01PM +0200, Andrew Lunn wrote:
> > > > /* Special struct emulating a Ethernet header */
> > > > struct qca_mgmt_ethhdr {
> > > > - u32 command; /* command bit 31:0 */
> > > > - u32 seq; /* seq 63:32 */
> > > > - u32 mdio_data; /* first 4byte mdio */
> > > > + __le32 command; /* command bit 31:0 */
> > > > + __le32 seq; /* seq 63:32 */
> > > > + __le32 mdio_data; /* first 4byte mdio */
> > > > __be16 hdr; /* qca hdr */
> > > > } __packed;
> > >
> > > It looks odd that hdr is BE while the rest are LE. Did you check this?
> > >
> > > Andrew
> >
> > Yes we did many test to analyze this and I just checked with some
> > tcpdump that the hdr is BE everytime.
>
> That might actual make sense. The comment says:
>
> > > > /* Special struct emulating a Ethernet header */
>
> And hdr is where the Ether type would be, which is network endian,
> i.e. big endian.
>
> Andrew
Yes that is my theory... hdr is in the ether type position so it's the
only part that the switch treat in a standard way as it has to be like
that or a dev creating a tagger driver would have no way to understand
if the packet is autocast, in band ack or a simple packet so who created
the fw for the switch had this concern in mind and stick to keeping at
least the hdr in a standard way.
--
Ansuel
On Wed, Oct 12, 2022 at 02:42:16PM +0200, Andrew Lunn wrote:
> On Mon, Oct 10, 2022 at 01:14:58PM +0200, Christian Marangi wrote:
> > The header and the data of the skb for the inband mgmt requires
> > to be in little-endian. This is problematic for big-endian system
> > as the mgmt header is written in the cpu byte order.
> >
> > Fix this by converting each value for the mgmt header and data to
> > little-endian, and convert to cpu byte order the mgmt header and
> > data sent by the switch.
> >
> > Fixes: 5950c7c0a68c ("net: dsa: qca8k: add support for mgmt read/write in Ethernet packet")
> > Tested-by: Pawel Dembicki <[email protected]>
> > Tested-by: Lech Perczak <[email protected]>
> > Signed-off-by: Christian Marangi <[email protected]>
> > Reviewed-by: Lech Perczak <[email protected]>
> > ---
> > drivers/net/dsa/qca/qca8k-8xxx.c | 63 ++++++++++++++++++++++++--------
> > include/linux/dsa/tag_qca.h | 6 +--
> > 2 files changed, 50 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> > index 5669c92c93f7..4bb9b7eac68b 100644
> > --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> > +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> > @@ -137,27 +137,42 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
> > struct qca8k_mgmt_eth_data *mgmt_eth_data;
> > struct qca8k_priv *priv = ds->priv;
> > struct qca_mgmt_ethhdr *mgmt_ethhdr;
> > + u32 command;
> > u8 len, cmd;
> > + int i;
> >
> > mgmt_ethhdr = (struct qca_mgmt_ethhdr *)skb_mac_header(skb);
> > mgmt_eth_data = &priv->mgmt_eth_data;
> >
> > - cmd = FIELD_GET(QCA_HDR_MGMT_CMD, mgmt_ethhdr->command);
> > - len = FIELD_GET(QCA_HDR_MGMT_LENGTH, mgmt_ethhdr->command);
> > + command = le32_to_cpu(mgmt_ethhdr->command);
> > + cmd = FIELD_GET(QCA_HDR_MGMT_CMD, command);
> > + len = FIELD_GET(QCA_HDR_MGMT_LENGTH, command);
>
> Humm...
>
> This might have the same alignment issue as the second patch. In fact,
> because the Ethernet header is 14 bytes in size, it is often
> deliberately out of alignment by 2 bytes, so that the IP header is
> aligned. You should probably be using get_unaligned_le32() when
> accessing members of mgmt_ethhdr.
>
> Andrew
Should I replace everything to get_unaligned_le32? Or this is only
needed for the mgmt_ethhdr as it's 12 bytes?
The skb data is all 32 bit contiguous stuff so it should be safe? Or
should we treat that also as unalligned just to make sure?
Same question for patch 2. the rest of the mib in skb data are all 32 or
64 values contiguous so wonder if we just take extra care of the
mgmt_ethhdr.
--
Ansuel
On Mon, Oct 10, 2022 at 01:14:58PM +0200, Christian Marangi wrote:
> The header and the data of the skb for the inband mgmt requires
> to be in little-endian. This is problematic for big-endian system
> as the mgmt header is written in the cpu byte order.
>
> Fix this by converting each value for the mgmt header and data to
> little-endian, and convert to cpu byte order the mgmt header and
> data sent by the switch.
>
> Fixes: 5950c7c0a68c ("net: dsa: qca8k: add support for mgmt read/write in Ethernet packet")
> Tested-by: Pawel Dembicki <[email protected]>
> Tested-by: Lech Perczak <[email protected]>
> Signed-off-by: Christian Marangi <[email protected]>
> Reviewed-by: Lech Perczak <[email protected]>
> ---
> drivers/net/dsa/qca/qca8k-8xxx.c | 63 ++++++++++++++++++++++++--------
> include/linux/dsa/tag_qca.h | 6 +--
> 2 files changed, 50 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index 5669c92c93f7..4bb9b7eac68b 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -137,27 +137,42 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
> struct qca8k_mgmt_eth_data *mgmt_eth_data;
> struct qca8k_priv *priv = ds->priv;
> struct qca_mgmt_ethhdr *mgmt_ethhdr;
> + u32 command;
> u8 len, cmd;
> + int i;
>
> mgmt_ethhdr = (struct qca_mgmt_ethhdr *)skb_mac_header(skb);
> mgmt_eth_data = &priv->mgmt_eth_data;
>
> - cmd = FIELD_GET(QCA_HDR_MGMT_CMD, mgmt_ethhdr->command);
> - len = FIELD_GET(QCA_HDR_MGMT_LENGTH, mgmt_ethhdr->command);
> + command = le32_to_cpu(mgmt_ethhdr->command);
> + cmd = FIELD_GET(QCA_HDR_MGMT_CMD, command);
> + len = FIELD_GET(QCA_HDR_MGMT_LENGTH, command);
Humm...
This might have the same alignment issue as the second patch. In fact,
because the Ethernet header is 14 bytes in size, it is often
deliberately out of alignment by 2 bytes, so that the IP header is
aligned. You should probably be using get_unaligned_le32() when
accessing members of mgmt_ethhdr.
Andrew
On Wed, Oct 12, 2022 at 02:54:54PM +0200, Christian Marangi wrote:
> On Wed, Oct 12, 2022 at 02:42:16PM +0200, Andrew Lunn wrote:
> > On Mon, Oct 10, 2022 at 01:14:58PM +0200, Christian Marangi wrote:
> > > The header and the data of the skb for the inband mgmt requires
> > > to be in little-endian. This is problematic for big-endian system
> > > as the mgmt header is written in the cpu byte order.
> > >
> > > Fix this by converting each value for the mgmt header and data to
> > > little-endian, and convert to cpu byte order the mgmt header and
> > > data sent by the switch.
> > >
> > > Fixes: 5950c7c0a68c ("net: dsa: qca8k: add support for mgmt read/write in Ethernet packet")
> > > Tested-by: Pawel Dembicki <[email protected]>
> > > Tested-by: Lech Perczak <[email protected]>
> > > Signed-off-by: Christian Marangi <[email protected]>
> > > Reviewed-by: Lech Perczak <[email protected]>
> > > ---
> > > drivers/net/dsa/qca/qca8k-8xxx.c | 63 ++++++++++++++++++++++++--------
> > > include/linux/dsa/tag_qca.h | 6 +--
> > > 2 files changed, 50 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> > > index 5669c92c93f7..4bb9b7eac68b 100644
> > > --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> > > +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> > > @@ -137,27 +137,42 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
> > > struct qca8k_mgmt_eth_data *mgmt_eth_data;
> > > struct qca8k_priv *priv = ds->priv;
> > > struct qca_mgmt_ethhdr *mgmt_ethhdr;
> > > + u32 command;
> > > u8 len, cmd;
> > > + int i;
> > >
> > > mgmt_ethhdr = (struct qca_mgmt_ethhdr *)skb_mac_header(skb);
> > > mgmt_eth_data = &priv->mgmt_eth_data;
> > >
> > > - cmd = FIELD_GET(QCA_HDR_MGMT_CMD, mgmt_ethhdr->command);
> > > - len = FIELD_GET(QCA_HDR_MGMT_LENGTH, mgmt_ethhdr->command);
> > > + command = le32_to_cpu(mgmt_ethhdr->command);
> > > + cmd = FIELD_GET(QCA_HDR_MGMT_CMD, command);
> > > + len = FIELD_GET(QCA_HDR_MGMT_LENGTH, command);
> >
> > Humm...
> >
> > This might have the same alignment issue as the second patch. In fact,
> > because the Ethernet header is 14 bytes in size, it is often
> > deliberately out of alignment by 2 bytes, so that the IP header is
> > aligned. You should probably be using get_unaligned_le32() when
> > accessing members of mgmt_ethhdr.
> >
> > Andrew
>
> Should I replace everything to get_unaligned_le32? Or this is only
> needed for the mgmt_ethhdr as it's 12 bytes?
>
> The skb data is all 32 bit contiguous stuff so it should be safe? Or
> should we treat that also as unalligned just to make sure?
>
> Same question for patch 2. the rest of the mib in skb data are all 32 or
> 64 values contiguous so wonder if we just take extra care of the
> mgmt_ethhdr.
>
Also also... Should I use put_unalligned to fill the mgmt_ethhdr?
--
Ansuel
On Wed, Oct 12, 2022 at 02:59:17PM +0200, Christian Marangi wrote:
> > > Humm...
> > >
> > > This might have the same alignment issue as the second patch. In fact,
> > > because the Ethernet header is 14 bytes in size, it is often
> > > deliberately out of alignment by 2 bytes, so that the IP header is
> > > aligned. You should probably be using get_unaligned_le32() when
> > > accessing members of mgmt_ethhdr.
> > >
> > > Andrew
> >
> > Should I replace everything to get_unaligned_le32? Or this is only
> > needed for the mgmt_ethhdr as it's 12 bytes?
> >
> > The skb data is all 32 bit contiguous stuff so it should be safe? Or
> > should we treat that also as unalligned just to make sure?
> >
> > Same question for patch 2. the rest of the mib in skb data are all 32 or
> > 64 values contiguous so wonder if we just take extra care of the
> > mgmt_ethhdr.
> >
>
> Also also... Should I use put_unalligned to fill the mgmt_ethhdr?
Documentation/core-api/unaligned-memory-access.rst section "Alignment vs. Networking"
says that the IP header is aligned to a 4 byte boundary.
Relative to the IP header, skb_mac_header(skb) is a pointer that's 14
bytes behind, right?
14 bytes behind something aligned to a 4 byte boundary is something
that's not aligned to a 4 byte boundary. That's why Andrew is suggesting
to use the unaligned helper for accesses (both put and get).
On-stack data structures don't need this, the compiler should take care
of aligning them and their fields appropriately. The trouble is with
pointers generated using manual arithmetics.