diff mbox series

[net,2/5] bnxt_en: Fix devlink NVRAM related byte order related issues.

Message ID 1571636069-14179-3-git-send-email-michael.chan@broadcom.com
State Accepted
Delegated to: David Miller
Headers show
Series bnxt_en: Bug fixes. | expand

Commit Message

Michael Chan Oct. 21, 2019, 5:34 a.m. UTC 
The current code does not do endian swapping between the devlink
parameter and the internal NVRAM representation.  Define a union to
represent the little endian NVRAM data and add 2 helper functions to
copy to and from the NVRAM data with the proper byte swapping.

Fixes: 782a624d00fa ("bnxt_en: Add bnxt_en initial port params table and register it")
Cc: Jiri Pirko <jiri@mellanox.com>
Reviewed-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 81 +++++++++++++++--------
 1 file changed, 54 insertions(+), 27 deletions(-)

Comments

Jakub Kicinski Oct. 22, 2019, 4:14 a.m. UTC | #1 
On Mon, 21 Oct 2019 01:34:26 -0400, Michael Chan wrote:
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> index 68f74f5..bd4b9f3 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> @@ -226,12 +226,55 @@ static const struct bnxt_dl_nvm_param nvm_params[] = {
>  	 BNXT_NVM_SHARED_CFG, 1, 1},
>  };
>  
> +union bnxt_nvm_data {
> +	u8	val8;
> +	__le32	val32;
> +};
> +
> +static void bnxt_copy_to_nvm_data(union bnxt_nvm_data *dst,
> +				  union devlink_param_value *src,
> +				  int nvm_num_bits, int dl_num_bytes)
> +{
> +	u32 val32 = 0;
> +
> +	if (nvm_num_bits == 1) {
> +		dst->val8 = src->vbool;
> +		return;
> +	}

Why do you special case the num_bits == 1? If val32 is __le32 the low
byte would have landed on the first byte anyway, no? 🤔

just curious

> +	if (dl_num_bytes == 4)
> +		val32 = src->vu32;
> +	else if (dl_num_bytes == 2)
> +		val32 = (u32)src->vu16;
> +	else if (dl_num_bytes == 1)
> +		val32 = (u32)src->vu8;
> +	dst->val32 = cpu_to_le32(val32);
> +}
Michael Chan Oct. 22, 2019, 5:38 a.m. UTC | #2 
On Mon, Oct 21, 2019 at 9:14 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Mon, 21 Oct 2019 01:34:26 -0400, Michael Chan wrote:
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> > index 68f74f5..bd4b9f3 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> > @@ -226,12 +226,55 @@ static const struct bnxt_dl_nvm_param nvm_params[] = {
> >        BNXT_NVM_SHARED_CFG, 1, 1},
> >  };
> >
> > +union bnxt_nvm_data {
> > +     u8      val8;
> > +     __le32  val32;
> > +};
> > +
> > +static void bnxt_copy_to_nvm_data(union bnxt_nvm_data *dst,
> > +                               union devlink_param_value *src,
> > +                               int nvm_num_bits, int dl_num_bytes)
> > +{
> > +     u32 val32 = 0;
> > +
> > +     if (nvm_num_bits == 1) {
> > +             dst->val8 = src->vbool;
> > +             return;
> > +     }
>
> Why do you special case the num_bits == 1? If val32 is __le32 the low
> byte would have landed on the first byte anyway, no?
>
> just curious

Just so that I don't have to do any casting.  Otherwise if I assign it
to the __le32, I believe I have to cast to avoid the warning.

>
> > +     if (dl_num_bytes == 4)
> > +             val32 = src->vu32;
> > +     else if (dl_num_bytes == 2)
> > +             val32 = (u32)src->vu16;
> > +     else if (dl_num_bytes == 1)
> > +             val32 = (u32)src->vu8;
> > +     dst->val32 = cpu_to_le32(val32);
> > +}
diff mbox series

Patch

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 68f74f5..bd4b9f3 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -226,12 +226,55 @@  static const struct bnxt_dl_nvm_param nvm_params[] = {
 	 BNXT_NVM_SHARED_CFG, 1, 1},
 };
 
+union bnxt_nvm_data {
+	u8	val8;
+	__le32	val32;
+};
+
+static void bnxt_copy_to_nvm_data(union bnxt_nvm_data *dst,
+				  union devlink_param_value *src,
+				  int nvm_num_bits, int dl_num_bytes)
+{
+	u32 val32 = 0;
+
+	if (nvm_num_bits == 1) {
+		dst->val8 = src->vbool;
+		return;
+	}
+	if (dl_num_bytes == 4)
+		val32 = src->vu32;
+	else if (dl_num_bytes == 2)
+		val32 = (u32)src->vu16;
+	else if (dl_num_bytes == 1)
+		val32 = (u32)src->vu8;
+	dst->val32 = cpu_to_le32(val32);
+}
+
+static void bnxt_copy_from_nvm_data(union devlink_param_value *dst,
+				    union bnxt_nvm_data *src,
+				    int nvm_num_bits, int dl_num_bytes)
+{
+	u32 val32;
+
+	if (nvm_num_bits == 1) {
+		dst->vbool = src->val8;
+		return;
+	}
+	val32 = le32_to_cpu(src->val32);
+	if (dl_num_bytes == 4)
+		dst->vu32 = val32;
+	else if (dl_num_bytes == 2)
+		dst->vu16 = (u16)val32;
+	else if (dl_num_bytes == 1)
+		dst->vu8 = (u8)val32;
+}
+
 static int bnxt_hwrm_nvm_req(struct bnxt *bp, u32 param_id, void *msg,
 			     int msg_len, union devlink_param_value *val)
 {
 	struct hwrm_nvm_get_variable_input *req = msg;
-	void *data_addr = NULL, *buf = NULL;
 	struct bnxt_dl_nvm_param nvm_param;
+	union bnxt_nvm_data *data;
 	dma_addr_t data_dma_addr;
 	int idx = 0, rc, i;
 
@@ -254,26 +297,9 @@  static int bnxt_hwrm_nvm_req(struct bnxt *bp, u32 param_id, void *msg,
 	else if (nvm_param.dir_type == BNXT_NVM_FUNC_CFG)
 		idx = bp->pf.fw_fid - BNXT_FIRST_PF_FID;
 
-	switch (nvm_param.dl_num_bytes) {
-	case 1:
-		if (nvm_param.nvm_num_bits == 1)
-			buf = &val->vbool;
-		else
-			buf = &val->vu8;
-		break;
-	case 2:
-		buf = &val->vu16;
-		break;
-	case 4:
-		buf = &val->vu32;
-		break;
-	default:
-		return -EFAULT;
-	}
-
-	data_addr = dma_alloc_coherent(&bp->pdev->dev, nvm_param.dl_num_bytes,
-				       &data_dma_addr, GFP_KERNEL);
-	if (!data_addr)
+	data = dma_alloc_coherent(&bp->pdev->dev, sizeof(*data),
+				  &data_dma_addr, GFP_KERNEL);
+	if (!data)
 		return -ENOMEM;
 
 	req->dest_data_addr = cpu_to_le64(data_dma_addr);
@@ -284,17 +310,18 @@  static int bnxt_hwrm_nvm_req(struct bnxt *bp, u32 param_id, void *msg,
 		req->dimensions = cpu_to_le16(1);
 
 	if (req->req_type == cpu_to_le16(HWRM_NVM_SET_VARIABLE)) {
-		memcpy(data_addr, buf, nvm_param.dl_num_bytes);
+		bnxt_copy_to_nvm_data(data, val, nvm_param.nvm_num_bits,
+				      nvm_param.dl_num_bytes);
 		rc = hwrm_send_message(bp, msg, msg_len, HWRM_CMD_TIMEOUT);
 	} else {
 		rc = hwrm_send_message_silent(bp, msg, msg_len,
 					      HWRM_CMD_TIMEOUT);
+		if (!rc)
+			bnxt_copy_from_nvm_data(val, data,
+						nvm_param.nvm_num_bits,
+						nvm_param.dl_num_bytes);
 	}
-	if (!rc && req->req_type == cpu_to_le16(HWRM_NVM_GET_VARIABLE))
-		memcpy(buf, data_addr, nvm_param.dl_num_bytes);
-
-	dma_free_coherent(&bp->pdev->dev, nvm_param.dl_num_bytes, data_addr,
-			  data_dma_addr);
+	dma_free_coherent(&bp->pdev->dev, sizeof(*data), data, data_dma_addr);
 	if (rc == -EACCES)
 		netdev_err(bp->dev, "PF does not have admin privileges to modify NVM config\n");
 	return rc;