linux-kernel - Re: [PATCH V2 3/4] 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]

Date:   Sat, 26 Jan 2019 00:10:41 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Sowjanya Komatineni <skomatineni@...dia.com>
Cc:     Dmitry Osipenko <digetx@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Mantravadi Karthik <mkarthik@...dia.com>,
        Shardar Mohammed <smohammed@...dia.com>,
        Timo Alho <talho@...dia.com>,
        "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>
Subject: Re: [PATCH V2 3/4] i2c: tegra: Add DMA Support

On Fri, Jan 25, 2019 at 09:11:54PM +0000, Sowjanya Komatineni wrote:
> 
> 
> > >>> +	if (i2c_dev->has_dma) {
> > >>> +		ret = tegra_i2c_init_dma_param(i2c_dev, true);
> > >>> +		if (ret == -EPROBE_DEFER)
> > >>> +			goto disable_div_clk;
> > >>> +		ret = tegra_i2c_init_dma_param(i2c_dev, false);
> > >>> +		if (ret == -EPROBE_DEFER)
> > >>> +			goto disable_div_clk;
> > >>
> > >> So tegra_i2c_init_dma_param() could fail, printing a error
> > >> message, and probe will succeed? If allocation fails during the
> > >> driver's probe, then just fail the probe. Please give the
> > >> rationale.
> > > 
> > > If APB DMA probe doesn’t happen prior to tegra i2c, DMA is not
> > > available by the time tegra_init_dma_param tries to request slave
> > > channel and in those cases dma_request_slave_channel_reason will
> > > return EPROBE_DEFER for tegra I2C probe to retry
> > > 
> > > In case if DMA is available but DMA buffer allocation fails, then
> > > tegra_i2c_init_dma_param returns ENOMEM and probe also fails
> > > returning same ENOMEM
> >
> > Is that what you're going to change in the next version? Your
> > current variant of the code doesn't fail the probe on ENOMEM and
> > there is duplicated attempt to invoke tegra_i2c_init_dma_param()
> > during the transfer.
> 
> Sorry correction to my previous reply.  If DMA buffer allocation
> fails, tegra_i2c_init_dma_param returns ENOMEM but probe will succeed
> as i2c transaction need to happen during  boot for some platform
> device programming for successful boot and they use PIO mode as xfer
> bytes is less and deferring i2c probe for ENOMEM causes boot to fail
> so during probe EPROBE_DEFER is only taken care.
> 
> Re-attempt of tegra_i2c_init_dma_param in xfer happens only if no
> successful DMA channel allocation happens prior to that ( during probe
> in case of ENOMEM).
> DMA mode is mainly for large transfer, and i2c xfer returning failure
> due to failing DMA buffer allocation causes boot to hang as platform
> device programming need to happen which doesn’t need to use DMA.
> Will fix this and will send updated patch to reattempt DMA request and
> buffer allocation during DMA mode transfer and will return fail for
> DMA mode I2C transfer...

I'm wondering if we shouldn't gracefully to PIO if we fail to allocate
DMA buffers. Even if it is unlikely that large transfers (> than the
threshold that activates the DMA paths) will be needed by critical I2C
clients, there's really no reason why we can't do PIO if DMA doesn't
work for some reason.

Thierry

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