[PATCH 1/2] mtd: spinand: Introduce a way to avoid raw access

[Miquel Raynal]

On 13/11/2024 at 11:06:21 +09, Takahiro Kuwano <tkuw584924 at gmail.com> wrote:

> Hi,
>
> On 11/12/2024 3:54 AM, Miquel Raynal wrote:
>> Hi,
>> 
>> On 31/10/2024 at 11:21:54 +09, tkuw584924 at gmail.com wrote:
>> 
>>> From: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>>>
>>> SkyHigh spinand device has ECC enable bit in configuration register but
>>> it must be always enabled. If ECC is disabled, read and write ops
>>> results in undetermined state. For such devices, a way to avoid raw
>>> access is needed.
>>>
>>> Introduce SPINAND_NO_RAW_ACCESS flag to advertise the device does not
>>> support raw access. Read and write page in raw mode for the device
>>> returns error.
>>>
>>> Checking and marking BBM need to be performed with ECC enabled to read
>>> and write the BBM correctly.
>> 
>> I see your point but I'm a bit puzzled by how it's being done.
>> 
>> First, you disregard completely the isbad() and markbad()
>> situations. Please have look into that because these functions are
>> broken with your devices.
>> 
> Yes... now I understand how they are broken.
>
>> Second, what about adding this detail to the ondie ECC engine? You could
>> simply return an error from there, so basically a single (or maybe two)
>> changes overall.
>> 
> Thank you for the suggestion.
> How about change like below in prepare_finish_io_req()?
>
> static int spinand_ondie_ecc_prepare_io_req(struct nand_device *nand,
> 					    struct nand_page_io_req *req)
> {
> 	struct spinand_device *spinand = nand_to_spinand(nand);
> 	bool enable = (req->mode != MTD_OPS_RAW);
>
> +	/*
> +	 * For the devices that prohibit raw access (on-die ECC must be always
> +	 * enabled), return error in case of raw data access. Accessing to OOB
> +	 * needs to be allowed with on-die ECC enabled to support BBM checking
> +	 * and marking.
> +	 */
> +	if (!enable && spinand->flags & SPINAND_NO_RAW_ACCESS) {
> +		if (req->datalen)
> +			return -ENOTSUPP;

EOPNOTSUPP

> +
> +		enable = true;
> +	}

No, this is lying, I don't like that.

What about just returnint an error here if you cannot do what the upper
layer has asked you to do. And in the upper layer, you can add two
specific conditions to is_bad/mark_bad() allowing to fallback to a "with
ECC" operation in the EOPNOTSUPP case (with a short comment).

Thanks,
Miquèl