linux-kernel - Re: [PATCH v2 1/1] mtd: spi-nor: core: replace dummy buswidth from addr to data

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: <mafs0ldvd5l2k.fsf@kernel.org>
Date: Tue, 14 Jan 2025 18:04:03 +0000
From: Pratyush Yadav <pratyush@...nel.org>
To: Miquel Raynal <miquel.raynal@...tlin.com>
Cc: Pratyush Yadav <pratyush@...nel.org>,  Alexander Stein
 <alexander.stein@...tq-group.com>,  tudor.ambarus@...aro.org,
  mwalle@...nel.org,  richard@....at,  vigneshr@...com,
  linux-mtd@...ts.infradead.org,  linux-kernel@...r.kernel.org,
  alvinzhou@...c.com.tw,  leoyu@...c.com.tw,  Cheng Ming Lin
 <chengminglin@...c.com.tw>,  stable@...r.kernel.org,  Cheng Ming Lin
 <linchengming884@...il.com>
Subject: Re: [PATCH v2 1/1] mtd: spi-nor: core: replace dummy buswidth from
 addr to data

On Tue, Jan 14 2025, Miquel Raynal wrote:

> Hello Pratyush,
>
> On 14/01/2025 at 16:15:24 GMT, Pratyush Yadav <pratyush@...nel.org> wrote:
>
>>>> --- a/drivers/mtd/spi-nor/core.c
>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>> @@ -89,7 +89,7 @@ void spi_nor_spimem_setup_op(const struct spi_nor *nor,
>>>>  		op->addr.buswidth = spi_nor_get_protocol_addr_nbits(proto);
>>>>  
>>>>  	if (op->dummy.nbytes)
>>>> -		op->dummy.buswidth = spi_nor_get_protocol_addr_nbits(proto);
>>>> +		op->dummy.buswidth = spi_nor_get_protocol_data_nbits(proto);
>
> Facing recently a similar issue myself in the SPI NAND world, I believe
> we should get rid of the notion of bits when it comes to the dummy
> phase. I would appreciate a clarification like "dummy.cycles" which
> would typically not require any bus width implications.

I agree. All peripheral drivers convert cycles to bytes, and controller
drivers convert them back to cycles. This whole thing should be avoided,
especially since it contains some traps with division truncation.

>
> ...
>
>> Most controller's supports_op hook call spi_mem_default_supports_op(),
>> including nxp_fspi_supports_op(). In spi_mem_default_supports_op(),
>> spi_mem_check_buswidth() is called to check if the buswidths for the op
>> can actually be supported by the board's wiring. This wiring information
>> comes from (among other things) the spi-{tx,rx}-bus-width DT properties.
>> Based on these properties, SPI_TX_* or SPI_RX_* flags are set by
>> of_spi_parse_dt(). spi_mem_check_buswidth() then uses these flags to
>> make the decision whether an op can be supported by the board's wiring
>> (in a way, indirectly checking against spi-{rx,tx}-bus-width).
>
> Thanks for the whole explanation, it's pretty clear.
>
>> In arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql.dtsi we have:
>>
>> 	flash0: flash@0 {
>> 		reg = <0>;
>> 		compatible = "jedec,spi-nor";
>> 		spi-max-frequency = <80000000>;
>> 		spi-tx-bus-width = <1>;
>> 		spi-rx-bus-width = <4>;
>>
>> Now the tricky bit here is we do the below in spi_mem_check_buswidth():
>>
>> 	if (op->dummy.nbytes &&
>> 	    spi_check_buswidth_req(mem, op->dummy.buswidth, true))
>> 		return false;
>
> May I challenge this entire section? Is there *any* reason to check
> anything against dummy cycles wrt the width? Maybe a "can handle x
> cycles" check would be interesting though, but I'd go for a different
> helper, that is specific to the dummy cycles.

I suppose you would want to sanity check that the cycles are at least
between 1, 2, 4, or 8 (or at the very least not 0).

>
>> The "true" parameter here means to "treat the op as TX". Since the
>> board only supports 1-bit TX, the 4-bit dummy TX is considered as
>> unsupported, and the op gets rejected. In reality, a dummy phase is
>> neither a RX nor a TX. We should ideally treat it differently, and
>> only check if it is one of 1, 2, 4, or 8, and not test it against the
>> board capabilities at all.
>
> ...
>
>> Since we are quite late in the cycle, and that changing
>> spi_mem_check_buswidth() might cause all sorts of breakages, I think the
>> best idea currently would be to revert this patch, and resend it with
>> the other changes later.
>>
>> Tudor, Michael, Miquel, what do you think about this? We are at rc7 but
>> I think we should send out a fixes PR with a revert. If you agree, I
>> will send out a patch and a PR.
>
> Either way I am fine. the -rc cycles are also available for us to
> settle. But it's true we can bikeshed a little bit, so feel free to
> revert this patch before sending the MR.

To be clear, since the patch was added in v6.13-rc1 I want to revert it
via a fixes pull request to Linus before he releases v6.13 this week. I
want to fix it in v6.13, not in v6.14.

-- 
Regards,
Pratyush Yadav