The set_raw_ingress_record() calls aq_mss_mdio_write() but does not check the return value. A proper implementation can be found in get_raw_ingress_record().
Add error handling for aq_mss_mdio_write(). If the write fails, return immediately.
Fixes: b8f8a0b7b5cb ("net: atlantic: MACSec ingress offload HW bindings") Cc: stable@vger.kernel.org # v5.7 Signed-off-by: Wentao Liang vulab@iscas.ac.cn --- .../aquantia/atlantic/macsec/macsec_api.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/aquantia/atlantic/macsec/macsec_api.c b/drivers/net/ethernet/aquantia/atlantic/macsec/macsec_api.c index 431924959520..5e87f8b749c5 100644 --- a/drivers/net/ethernet/aquantia/atlantic/macsec/macsec_api.c +++ b/drivers/net/ethernet/aquantia/atlantic/macsec/macsec_api.c @@ -62,6 +62,7 @@ static int set_raw_ingress_record(struct aq_hw_s *hw, u16 *packed_record, { struct mss_ingress_lut_addr_ctl_register lut_sel_reg; struct mss_ingress_lut_ctl_register lut_op_reg; + int ret;
unsigned int i;
@@ -105,11 +106,15 @@ static int set_raw_ingress_record(struct aq_hw_s *hw, u16 *packed_record, lut_op_reg.bits_0.lut_read = 0; lut_op_reg.bits_0.lut_write = 1;
- aq_mss_mdio_write(hw, MDIO_MMD_VEND1, - MSS_INGRESS_LUT_ADDR_CTL_REGISTER_ADDR, - lut_sel_reg.word_0); - aq_mss_mdio_write(hw, MDIO_MMD_VEND1, MSS_INGRESS_LUT_CTL_REGISTER_ADDR, - lut_op_reg.word_0); + ret = aq_mss_mdio_write(hw, MDIO_MMD_VEND1, + MSS_INGRESS_LUT_ADDR_CTL_REGISTER_ADDR, + lut_sel_reg.word_0); + if (unlikely(ret)) + return ret; + ret = aq_mss_mdio_write(hw, MDIO_MMD_VEND1, MSS_INGRESS_LUT_CTL_REGISTER_ADDR, + lut_op_reg.word_0); + if (unlikely(ret)) + return ret;
return 0; }
The set_raw_ingress_record() calls aq_mss_mdio_write() but does not check the return value. A proper implementation can be found in get_raw_ingress_record(). Add error handling for aq_mss_mdio_write(). If the write fails, return immediately.
The set_raw_ingress_record() calls aq_mss_mdio_write() but does not check the return value. A proper implementation can be found in get_raw_ingress_record().
Add error handling for aq_mss_mdio_write(). If the write fails, return immediately.
Hi Wentao,
aq_mss_mdio_write is practically always returns success as of today.
So introducing your change without actually making aq_mss_mdio_write and aq_mdio_write_word returning error code is useless IMHO.
- ret = aq_mss_mdio_write(hw, MDIO_MMD_VEND1,
MSS_INGRESS_LUT_ADDR_CTL_REGISTER_ADDR,lut_sel_reg.word_0);- if (unlikely(ret))
return ret;- ret = aq_mss_mdio_write(hw, MDIO_MMD_VEND1, MSS_INGRESS_LUT_CTL_REGISTER_ADDR,
lut_op_reg.word_0);- if (unlikely(ret))
return ret;
You introduce checks only for the two last operations, but not for many others above them. Any reason to do so?
Overall, I think if you want to improve in this direction: 1) we should propagate error conditions from aq_mdio_write_word 2) all instances of aq_mss_mdio_write should have checks for err condition 3) this is not actually a bug, but improvement - therefore make sense to post this into net-next.
Regards, Igor
On Mon, May 19, 2025 at 06:21:32PM +0800, Wentao Liang wrote:
The set_raw_ingress_record() calls aq_mss_mdio_write() but does not check the return value. A proper implementation can be found in get_raw_ingress_record().
Add error handling for aq_mss_mdio_write(). If the write fails, return immediately.
Fixes: b8f8a0b7b5cb ("net: atlantic: MACSec ingress offload HW bindings") Cc: stable@vger.kernel.org # v5.7 Signed-off-by: Wentao Liang vulab@iscas.ac.cn
.../aquantia/atlantic/macsec/macsec_api.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/aquantia/atlantic/macsec/macsec_api.c b/drivers/net/ethernet/aquantia/atlantic/macsec/macsec_api.c index 431924959520..5e87f8b749c5 100644 --- a/drivers/net/ethernet/aquantia/atlantic/macsec/macsec_api.c +++ b/drivers/net/ethernet/aquantia/atlantic/macsec/macsec_api.c @@ -62,6 +62,7 @@ static int set_raw_ingress_record(struct aq_hw_s *hw, u16 *packed_record, { struct mss_ingress_lut_addr_ctl_register lut_sel_reg; struct mss_ingress_lut_ctl_register lut_op_reg;
- int ret;
unsigned int i; @@ -105,11 +106,15 @@ static int set_raw_ingress_record(struct aq_hw_s *hw, u16 *packed_record, lut_op_reg.bits_0.lut_read = 0; lut_op_reg.bits_0.lut_write = 1;
- aq_mss_mdio_write(hw, MDIO_MMD_VEND1,
MSS_INGRESS_LUT_ADDR_CTL_REGISTER_ADDR,lut_sel_reg.word_0);- aq_mss_mdio_write(hw, MDIO_MMD_VEND1, MSS_INGRESS_LUT_CTL_REGISTER_ADDR,
lut_op_reg.word_0);
- ret = aq_mss_mdio_write(hw, MDIO_MMD_VEND1,
MSS_INGRESS_LUT_ADDR_CTL_REGISTER_ADDR,lut_sel_reg.word_0);- if (unlikely(ret))
return ret;
What about the comment above:
/* NOTE: MSS registers must always be read/written as adjacent pairs. * For instance, to write either or both 1E.80A0 and 80A1, we have to: * 1. Write 1E.80A0 first * 2. Then write 1E.80A1
If the first write where to fail, is it better to perform the second write anyway, just to keep to this rule?
Andrew
linux-stable-mirror@lists.linaro.org
-
Andrew Lunn -
Igor Russkikh -
Wentao Liang