linux-kernel - Re: [PATCH V7 3/5] i2c: tegra: Add DMA Support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190131120640.GF23438@ulmo>
Date:   Thu, 31 Jan 2019 13:06:40 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     Sowjanya Komatineni <skomatineni@...dia.com>, jonathanh@...dia.com,
        mkarthik@...dia.com, smohammed@...dia.com, talho@...dia.com,
        linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-i2c@...r.kernel.org
Subject: Re: [PATCH V7 3/5] i2c: tegra: Add DMA Support

On Thu, Jan 31, 2019 at 03:05:48AM +0300, Dmitry Osipenko wrote:
> 30.01.2019 19:01, Sowjanya Komatineni пишет:
[...]
> > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
[...]
> > +		return -EIO;
> > +	}
> > +
> > +	dma_desc->callback = tegra_i2c_dma_complete;
> > +	dma_desc->callback_param = i2c_dev;
> > +	dmaengine_submit(dma_desc);
> > +	dma_async_issue_pending(chan);
> > +	return 0;
> > +}
> > +
> > +static int tegra_i2c_init_dma_param(struct tegra_i2c_dev *i2c_dev,
> > +				    bool dma_to_memory)
> > +{
> > +	struct dma_chan *dma_chan;
> > +	u32 *dma_buf;
> > +	dma_addr_t dma_phys;
> > +	int ret;
> > +	const char *chan_name = dma_to_memory ? "rx" : "tx";
> 
> What about to move out chan_name into the function arguments?

That opens up the possibility of passing dma_to_memory = true and
chan_name as "tx" and create an inconsistency.

> > @@ -884,6 +1187,8 @@ static void tegra_i2c_parse_dt(struct tegra_i2c_dev *i2c_dev)
> >  
> >  	i2c_dev->is_multimaster_mode = of_property_read_bool(np,
> >  			"multi-master");
> > +
> > +	i2c_dev->has_dma = of_property_read_bool(np, "dmas");
> 
> Not only the existence of "dmas" property defines whether DMA is available. DMA subsystem could be disabled in the kernels configuration.
> 
> Hence there is a need to check for DMA driver presence in the code:
> 
> 	if (IS_ENABLED(CONFIG_TEGRA20_APB_DMA))
> 		i2c_dev->has_dma = of_property_read_bool(np, "dmas");

Do we even need the ->has_dma at all? We can just go ahead and request
the channels at probe time and respond accordingly. If there's no dmas
property in DT, dma_request_slave_channel_reason() returns an error so
we can just deal with it at that time.

So if we get -EPROBE_DEFER we can propagate that, for any other errors
we can simply fallback to PIO. Or perhaps we want to restrict fallback
to PIO for -ENODEV?

I wouldn't want to add an IS_ENABLED(CONFIG_TEGRA20_APB_DMA) in here.
The purpose of these subsystems it to abstract all of that away.
Otherwise we could just as well use custom APIs, if we're tying together
drivers in this way anyway.

> Also Tegra I2C driver should select DMA driver in Kconfig to make DMA
> driver built-in when I2C driver is built-in:

I don't think there's a requirement for that. The only dependency we
really have here is the one on the DMA engine API. Since dmaengine.h
already provides dummy implementations, there's really no need for
us to have the dependency. If the DMA engine API is completely disabled,
a call to dma_request_slave_channel_reason() will return -ENODEV and we
should just deal with that the same way we would if there was no "dmas"
property present.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)