Re: [PATCH v3 17/27] mtd: spi-nor: debugfs: Add locking support

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Takahiro,

>> +u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor);
>> +void spi_nor_get_locked_range_sr(struct spi_nor *nor, const u8 *sr, loff_t *ofs, u64 *len);
>> +bool spi_nor_is_locked_sr(struct spi_nor *nor, loff_t ofs, u64 len, const u8 *sr);
>> +
>
> It would be better to have generic helper functions rather than using SR-based
> functions directly. The locking_ops is vendor/chip specific and can provide
> other locking mechanism than SR-based block protection. For instance, Infineon,
> Micron, Macronix (and maybe other vendors) offer protection mechanisms that can
> protect sectors individually with volatile or non-volatile manner.

I get what you mean, thanks for pointing this out. Unfortunately it's
not that straightforward.

As of today, there are the "default" locking ops (SR based) and a few
others, chip specific. For debugging purposes, these "other" ops do not
provide any kind of useful feedback regarding what has actually been
locked. Only SR based chips provide this, it is useful, I want to use it.

So I'm going to expose a new boolean to filter out whether we have
locking support *and* if yes, whether we can use SR based functions, this
should also address this concern:

> Don't we need to check 'SNOR_F_HAS_LOCK' flag here?

We will still need to expose the SR specific helpers, though, but at
least we will no longer get inconsistencies with chips not featuring the
default SR approach.

>> +       seq_puts(s, "\nlocked sectors\n");
>> +       seq_puts(s, " region (in hex)   | status   | #blocks\n");
>> +       seq_puts(s, " ------------------+----------+--------\n");
>> +
>> +       spi_nor_get_locked_range_sr(nor, nor->dfs_sr_cache, &lock_start, &lock_length);
>> +       if (!lock_length || lock_length == params->size) {
>> +               seq_printf(s, " %08llx-%08llx | %s | %llu\n", 0ULL, params->size - 1,
>> +                          lock_length ? "  locked" : "unlocked", params->size / min_prot_len);
>
> div_u64() is needed. 
> I got undefined reference to `__aeabi_uldivmod' for my 32-bit ARM platform.
> Same for following four seq_printf().

Good catch, I will fix this too.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/