linux-kernel - Re: [PATCH 1/3] spi: spi-mem: add automatic poll status functions

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: <28acedfd-6cd5-a8ad-0182-e61c3c30c27d@foss.st.com>
Date:   Fri, 30 Apr 2021 16:22:34 +0200
From:   Patrice CHOTARD <patrice.chotard@...s.st.com>
To:     Mark Brown <broonie@...nel.org>, Pratyush Yadav <p.yadav@...com>
CC:     Miquel Raynal <miquel.raynal@...tlin.com>,
        Vignesh Raghavendra <vigneshr@...com>,
        Boris Brezillon <boris.brezillon@...labora.com>,
        <linux-mtd@...ts.infradead.org>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        <linux-spi@...r.kernel.org>,
        <linux-stm32@...md-mailman.stormreply.com>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <christophe.kerello@...s.st.com>
Subject: Re: [PATCH 1/3] spi: spi-mem: add automatic poll status functions

Hi Mark, Pratyush

On 4/26/21 6:51 PM, Mark Brown wrote:
> On Mon, Apr 26, 2021 at 09:56:12PM +0530, Pratyush Yadav wrote:
>> On 26/04/21 04:39PM, patrice.chotard@...s.st.com wrote:
> 
>>> + * spi_mem_poll_status() - Poll memory device status
>>> + * @mem: SPI memory device
>>> + * @op: the memory operation to execute
>>> + * @mask: status bitmask to ckeck
>>> + * @match: status expected value
> 
>> Technically, (status & mask) expected value. Dunno if that is obvious 
>> enough to not spell out explicitly.
> 
> Is it possible there's some situation where you're waiting for some bits
> to clear as well?
> 
Yes, we are waiting STATUS_BUSY bit to be cleared, see patch 2 which is making 
usage of this API.

>>> +		ret = ctlr->mem_ops->poll_status(mem, op, mask, match, timeout);
> 
> I'm not sure I like this name since it makes me think the driver is
> going to poll when really it's offloaded to the hardware, but I can't
> think of any better ideas either and it *is* what the hardware is going
> to be doing so meh.
> 
>> I wonder if it is better to let spi-mem core take care of the timeout 
>> part. On one hand it reduces code duplication on the driver side a 
>> little bit. Plus it makes sure drivers don't mess anything up with bad 
>> (or no) handling of the timeout. But on the other hand the interface 
>> becomes a bit awkward since you'd have to pass a struct completion 
>> around, and it isn't something particularly hard to get right either. 
>> What do you think?
> 
> We already have the core handling other timeouts.  We don't pass around
> completions but rather have an API function that the driver has to call
> when the operation completes, a similar pattern might work here.  Part

So, if i correctly understood, you make allusion to what is already done
in SPI core framework with spi_finalize_current_transfer() right ?

> of the thing with those APIs which I'm missing here is that this will
> just return -EOPNOTSUPP if the driver can't do the delay in hardware, I
> think it would be cleaner if this API were similar and the core dealt
> with doing the delay/poll on the CPU.  That way the users don't need to
> repeat the handling for the offload/non-offload cases.

Sorry, i didn't catch what you mean here. In PATCH 2, that's the case,
if spi_mem_poll_status() is not supported, the core is dealing with 
the delay/poll on the CPU in spinand_wait().

Patrice
Thanks


>