Re: [PATCH v2] net/ncsi: Add NCSI OEM command support

[Vijay Khemka <vijaykhemka@fb.com>]

Hi Justin,
Thanks for response. Please see my comments below.

-Vijay
    > > diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
    > > index 7567ca63aae2..2f98533eba46 100644
    > > --- a/net/ncsi/ncsi-cmd.c
    > > +++ b/net/ncsi/ncsi-cmd.c
    > > @@ -211,6 +211,26 @@ static int ncsi_cmd_handler_snfc(struct sk_buff *skb,
    > >  	return 0;
    > >  }
    > >  
    > > +static int ncsi_cmd_handler_oem(struct sk_buff *skb,
    > > +				struct ncsi_cmd_arg *nca)
    > > +{
    > > +	struct ncsi_cmd_oem_pkt *cmd;
    
    >OEM command doesn't not have the fixed data size. Should we use pointer instead?
    >struct ncsi_cmd_oem_pkt {
    >	struct ncsi_cmd_pkt_hdr cmd;         /* Command header    */
    >	__be32                  mfr_id;      /* Manufacture ID    */
    >	unsigned char           *data;       /* OEM Payload Data  */
    >};
    Yes, I agree that OEM command doesn't have fixed data but to map to skbuff structure, 
    I have defined above structure as per NCSI spec, We can certainly have a MAX_DATA_LEN define.

    > > +	unsigned int len;
    > > +
    > > +	len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;
    > > +	if (nca->payload < 26)
    > > +		len += 26;
    
    >Why does it add 26? I knew the other place in ncsi_alloc_command() is also add 26.
    >If it is less than 26, then it should be a fixed size of structure ncsi_cmd_pkt (46), right?
    It adds 26 because It has already assigned len to hdr+4 which is 16+4 = 20 bytes. By 
    adding 26 it makes it to 46. I am just being consistent with other portion of code.

    > > +	else
    > > +		len += nca->payload;
    > > +
    > > +	cmd = skb_put_zero(skb, len);
    > > +	cmd->mfr_id = nca->dwords[0];
    > > +	memcpy(cmd->data, &nca->dwords[1], nca->payload - 4);
    
    >Netlink request is using the new nca->data pointer to pass the data as the request payload
    >is not the same size and some command payload is bigger than 16 bytes.
    >Will you consider to use the same data pointer? So, we don't need to have a checking here.
    >If the command is used less than 16 bytes, we can simply assigned &nca->bytes[0] to it.
    To keep original structure, we can change 16 bytes to MAX_DATA_LEN. Or I don't see any issue in 
    Copying data from data pointer from nca but user needs to be aware if it is less than 16 bytes then 
    use bytes or use data pointer. To keep it simple, we should simply define MAX_LEN.
    
    > > diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
    > > index 91b4b66438df..1f338386810d 100644
    > > --- a/net/ncsi/ncsi-pkt.h
    > > +++ b/net/ncsi/ncsi-pkt.h
    > > @@ -151,6 +151,22 @@ struct ncsi_cmd_snfc_pkt {
    > >  	unsigned char           pad[22];
    > >  };
    > >  
    > > +/* OEM Request Command as per NCSI Specification */
    > > +struct ncsi_cmd_oem_pkt {
    > > +	struct ncsi_cmd_pkt_hdr cmd;         /* Command header    */
    > > +	__be32                  mfr_id;      /* Manufacture ID    */
    > > +	unsigned char           data[64];    /* OEM Payload Data  */
    > > +	__be32                  checksum;    /* Checksum          */
    > > +};
    > > +
    > > +/* OEM Response Packet as per NCSI Specification */
    > > +struct ncsi_rsp_oem_pkt {
    > > +	struct ncsi_rsp_pkt_hdr rsp;         /* Command header    */
    > > +	__be32                  mfr_id;      /* Manufacture ID    */
    > > +	unsigned char           data[64];    /* Payload data      */
    > > +	__be32                  checksum;    /* Checksum          */
    > > +};
    > > +
    
    >OEM command doesn't not have the fixed response data size too.
    >Should we use pointer instead?

    Here also we can define MAX_DATA_LEN because data pointer won't map to skb directly.