Re: [PATCH v4 2/2] dmaengine: tegra210-adma: check for adma max page

Thierry Reding
2025-02-05T13:24:49Z

[-- Attachment #1: Type: text/plain, Size: 3743 bytes --]

On Wed, Feb 05, 2025 at 06:28:34PM +0530, Mohan Kumar D wrote:
>
> On 05-02-2025 17:10, Jon Hunter wrote:
> >
> >
> > On 05/02/2025 03:31, Mohan Kumar D wrote:
> > > Have additional check for max channel page during the probe
> > > to cover if any offset overshoot happens due to wrong DT
> > > configuration.
> > >
> > > Fixes: 68811c928f88 ("dmaengine: tegra210-adma: Support channel page")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Mohan Kumar D <mkumard@nvidia.com>
> > > ---
> > >   drivers/dma/tegra210-adma.c | 7 ++++++-
> > >   1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/dma/tegra210-adma.c b/drivers/dma/tegra210-adma.c
> > > index a0bd4822ed80..801740ad8e0d 100644
> > > --- a/drivers/dma/tegra210-adma.c
> > > +++ b/drivers/dma/tegra210-adma.c
> > > @@ -83,7 +83,9 @@ struct tegra_adma;
> > >    * @nr_channels: Number of DMA channels available.
> > >    * @ch_fifo_size_mask: Mask for FIFO size field.
> > >    * @sreq_index_offset: Slave channel index offset.
> > > + * @max_page: Maximum ADMA Channel Page.
> > >    * @has_outstanding_reqs: If DMA channel can have outstanding
> > > requests.
> > > + * @set_global_pg_config: Global page programming.
> > >    */
> > >   struct tegra_adma_chip_data {
> > >       unsigned int (*adma_get_burst_config)(unsigned int burst_size);
> > > @@ -99,6 +101,7 @@ struct tegra_adma_chip_data {
> > >       unsigned int nr_channels;
> > >       unsigned int ch_fifo_size_mask;
> > >       unsigned int sreq_index_offset;
> > > +    unsigned int max_page;
> > >       bool has_outstanding_reqs;
> > >       void (*set_global_pg_config)(struct tegra_adma *tdma);
> > >   };
> > > @@ -854,6 +857,7 @@ static const struct tegra_adma_chip_data
> > > tegra210_chip_data = {
> > >       .nr_channels        = 22,
> > >       .ch_fifo_size_mask    = 0xf,
> > >       .sreq_index_offset    = 2,
> > > +    .max_page        = 0,
> > >       .has_outstanding_reqs    = false,
> > >       .set_global_pg_config    = NULL,
> > >   };
> > > @@ -871,6 +875,7 @@ static const struct tegra_adma_chip_data
> > > tegra186_chip_data = {
> > >       .nr_channels        = 32,
> > >       .ch_fifo_size_mask    = 0x1f,
> > >       .sreq_index_offset    = 4,
> > > +    .max_page        = 4,
> > >       .has_outstanding_reqs    = true,
> > >       .set_global_pg_config    = tegra186_adma_global_page_config,
> > >   };
> > > @@ -921,7 +926,7 @@ static int tegra_adma_probe(struct
> > > platform_device *pdev)
> > >               page_offset = res_page->start - res_base->start;
> > >               page_no = div_u64(page_offset, cdata->ch_base_offset);
> > >   -            if (WARN_ON(page_no == 0))
> > > +            if (WARN_ON(page_no == 0 || page_no > cdata->max_page))
> >
> > So no one should ever specify the 'page' region for Tegra210, correct?
> > If they did then this would always fail. I don't know if it is also
> > worth checking if someone has a 'page' region for a device that has
> > max_page == 0?
> Yes, DT binding specifies "page" should not be used for Tegra210 and above
> conditions takes care. I believe above condition should suffice.

Yes, if you get this wrong, the DT validation should already catch it.
We could always make it very clear by adding an explicit check and a
corresponding error message, but I'm not sure it's really worth it in
this case. If we do, then maybe we should add error messages to these
other cases as well.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]