Re: [PATCH net v1 2/3] octeon_ep: ensure dbell BADDR updation

[Simon Horman]

On Fri, Dec 12, 2025 at 12:23:01PM +0000, Vimlesh Kumar wrote:
> Make sure the OUT DBELL base address reflects the
> latest values written to it.
> 
> Fix:
> Add a wait until the OUT DBELL base address register
> is updated with the DMA ring descriptor address,
> and modify the setup_oq function to properly
> handle failures.
> 
> Fixes: 0807dc76f3bf5("octeon_ep: support Octeon CN10K devices")

Hi Vimlesh,

Thanks for your patch.

Some feedback from my side.
First, there is a space missing in the Fixes tag:

Fixes: 0807dc76f3bf ("octeon_ep: support Octeon CN10K devices")

> Signed-off-by: Sathesh Edara <sedara@xxxxxxxxxxx>
> Signed-off-by: Shinas Rasheed <srasheed@xxxxxxxxxxx>
> Signed-off-by: Vimlesh Kumar <vimleshk@xxxxxxxxxxx>

...

>  /* Setup registers for a PF mailbox */
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_cnxk_pf.c b/drivers/net/ethernet/marvell/octeon_ep/octep_cnxk_pf.c

...

> @@ -343,6 +344,23 @@ static void octep_setup_oq_regs_cnxk_pf(struct octep_device *oct, int oq_no)
>  			reg_val = octep_read_csr64(oct, CNXK_SDP_R_OUT_CONTROL(oq_no));
>  		} while (!(reg_val & CNXK_R_OUT_CTL_IDLE));
>  	}
> +	octep_write_csr64(oct, CNXK_SDP_R_OUT_WMARK(oq_no),  oq->max_count);
> +	/* Wait for WMARK to get applied */
> +	usleep_range(10, 15);
> +
> +	octep_write_csr64(oct, CNXK_SDP_R_OUT_SLIST_BADDR(oq_no), oq->desc_ring_dma);

Please line-wrap Networking code to 80 columns wide or less where it can
be done without reducing readability (which is the case here).

checkpatch.pl --max-line-length=80 should flag this.

> +	octep_write_csr64(oct, CNXK_SDP_R_OUT_SLIST_RSIZE(oq_no), oq->max_count);
> +	reg_ba_val = octep_read_csr64(oct, CNXK_SDP_R_OUT_SLIST_BADDR(oq_no));
> +	if (reg_ba_val != oq->desc_ring_dma) {
> +		do {
> +			if (reg_ba_val == UINT64_MAX)

I think that ULLONG_MAX here, rather than defining UINT64_MAX
elsewhere in this patch.

It might be better if the Kernel provided UINT64_MAX and friends.
But it doesn't. (And I'm sure there are many opinions on why.)

> +				return -1;

This should be a standard error code.
Perhaps -EFAULT?

> +			octep_write_csr64(oct, CNXK_SDP_R_OUT_SLIST_BADDR(oq_no),
> +					  oq->desc_ring_dma);
> +			octep_write_csr64(oct, CNXK_SDP_R_OUT_SLIST_RSIZE(oq_no), oq->max_count);
> +			reg_ba_val = octep_read_csr64(oct, CNXK_SDP_R_OUT_SLIST_BADDR(oq_no));
> +		} while (reg_ba_val != oq->desc_ring_dma);

I am concerned that this loop is unbounded.
Could some limit be placed on it?

...

> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.h b/drivers/net/ethernet/marvell/octeon_ep/octep_main.h
> index 81ac4267811c..76622cdf577d 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.h
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.h
> @@ -55,6 +55,10 @@
>  				  (iq_)->max_count - IQ_INSTR_PENDING(iq_); \
>  				})
>  
> +#ifndef UINT64_MAX
> +#define UINT64_MAX ((u64)(~((u64)0)))        /* 0xFFFFFFFFFFFFFFFF */
> +#endif
> +
>  /* PCI address space mapping information.
>   * Each of the 3 address spaces given by BAR0, BAR2 and BAR4 of
>   * Octeon gets mapped to different physical address spaces in

...

-- 
pw-bot: cr