Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 11 Apr 2023 18:48:14 -0700
From:   Jakub Kicinski <kuba@kernel.org>
To:     Roman Gushchin <roman.gushchin@linux.dev>
Cc:     netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        Rafal Ozieblo <rafalo@cadence.com>,
        Lars-Peter Clausen <lars@metafoo.de>
Subject: Re: [PATCH net] net: macb: fix a memory corruption in extended
 buffer descriptor mode
Message-ID: <20230411184814.5be340a8@kernel.org>
In-Reply-To: <20230407172402.103168-1-roman.gushchin@linux.dev>
References: <20230407172402.103168-1-roman.gushchin@linux.dev>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Fri,  7 Apr 2023 10:24:02 -0700 Roman Gushchin wrote:
> The problem is resolved by extending the MACB_RX_WADDR_SIZE
> in the extended mode.
> 
> Fixes: 7b4296148066 ("net: macb: Add support for PTP timestamps in DMA descriptors")
> Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
> Co-developed-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>  drivers/net/ethernet/cadence/macb.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index c1fc91c97cee..1b330f7cfc09 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -826,8 +826,13 @@ struct macb_dma_desc_ptp {
>  #define MACB_RX_USED_SIZE			1
>  #define MACB_RX_WRAP_OFFSET			1
>  #define MACB_RX_WRAP_SIZE			1
> +#ifdef MACB_EXT_DESC
> +#define MACB_RX_WADDR_OFFSET			3
> +#define MACB_RX_WADDR_SIZE			29
> +#else
>  #define MACB_RX_WADDR_OFFSET			2
>  #define MACB_RX_WADDR_SIZE			30
> +#endif

Changing register definition based on Kconfig seems a bit old school.

Where is the extended descriptor mode enabled? Is it always on if
Kconfig is set or can it be off for some platforms based on other
capabilities? Judging by macb_dma_desc_get_size() small descriptors 
can still be used even with EXT_DESC?

If I'm grepping correctly thru the painful macro magic this register 
is only used in macb_get_addr(). It'd seem a bit more robust to me 
to open code the extraction of the address based on bp->hw_dma_cap
in that one function.

In addition to maintainers please also CC Harini Katakam
<harini.katakam@xilinx.com> on v2.