RE: [EXTERNAL] Re: [PATCH v7 1/1] misc: mrvl-cn10k-dpi: add Octeon CN10K DPI administrative driver

[Vamsi Krishna Attunuru]

> -----Original Message-----
> From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Tuesday, June 4, 2024 9:23 PM
> To: Vamsi Krishna Attunuru <vattunuru@xxxxxxxxxxx>
> Cc: Jerin Jacob <jerinj@xxxxxxxxxxx>; Srujana Challa <schalla@xxxxxxxxxxx>;
> arnd@xxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: [EXTERNAL] Re: [PATCH v7 1/1] misc: mrvl-cn10k-dpi: add Octeon
> CN10K DPI administrative driver
> 
> Prioritize security for external emails: Confirm sender and content safety
> before clicking links or opening attachments
> 
> ----------------------------------------------------------------------
> On Mon, May 20, 2024 at 04:06:30AM -0700, Vamsi Attunuru wrote:
> > +union dpi_mbox_message {
> > +	u64 word[2];
> > +	struct {
> > +#if defined(__BIG_ENDIAN_BITFIELD)
> > +		/* SSO PF function */
> > +		u64 sso_pf_func  :16;
> > +		/* Aura of the command buffer */
> > +		u64 aura         :20;
> > +		/* Command buffer size in 8-byte words */
> > +		u64 csize        :16;
> > +		/* Command code */
> > +		u64 cmd          :4;
> > +		/* VF ID to configure */
> > +		u64 vfid         :8;
> > +		/* Reserved for future use */
> > +		u64 rsvd_85_127  :40;
> > +		/* Work queue completion status byte offset */
> > +		u64 wqecsoff     :7;
> > +		/* Work queue completion status enable */
> > +		u64 wqecs        :1;
> > +		/* NPA PF function */
> > +		u64 npa_pf_func  :16;
> > +#else
> > +		/* VF ID to configure */
> > +		u64 vfid         :8;
> > +		/* Command code */
> > +		u64 cmd          :4;
> > +		/* Command buffer size in 8-byte words */
> > +		u64 csize        :16;
> > +		/* Aura of the command buffer */
> > +		u64 aura         :20;
> > +		/* SSO PF function */
> > +		u64 sso_pf_func  :16;
> > +		/* NPA PF function */
> > +		u64 npa_pf_func  :16;
> > +		/* Work queue completion status enable */
> > +		u64 wqecs        :1;
> > +		/* Work queue completion status byte offset */
> > +		u64 wqecsoff     :7;
> > +		/* Reserved for future use */
> > +		u64 rsvd_85_127  :40;
> > +#endif
> > +	};
> > +};
> 
> The ifdef is cute, but not correct, sorry.  Please use bit shifts to handle this
> properly without any #ifdef needed at all.
> 
Ack, will fix it next version. Thanks for the suggestion.

> 
> 
> 
> > +
> > +static inline void dpi_reg_write(struct dpipf *dpi, u64 offset, u64
> > +val) {
> > +	writeq(val, dpi->reg_base + offset);
> 
> No read needed after a write to ensure the write made it to the hardware
> properly?

Yes, as it's an onboard PCIe device, writes will happen properly. I will modify it as write followed by
a write barrier to avoid any reordering.

Thanks
Vamsi
> 
> thanks,
> 
> greg k-h