Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Tue, 01 Jun 2021 15:02:24 +0200
From:   Michael Walle <michael@walle.cc>
To:     Tudor.Ambarus@microchip.com
Cc:     linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org,
        p.yadav@ti.com, miquel.raynal@bootlin.com, richard@nod.at,
        vigneshr@ti.com
Subject: Re: [PATCH v4 3/4] mtd: spi-nor: otp: return -EROFS if region is
 read-only
In-Reply-To: <bf3a9cf9-fc72-8ea4-de0a-1f0e123cb644@microchip.com>
References: <20210521194034.15249-1-michael@walle.cc>
 <20210521194034.15249-4-michael@walle.cc>
 <bf3a9cf9-fc72-8ea4-de0a-1f0e123cb644@microchip.com>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <e9a697de15dcf4f526a7f2da95b67b73@walle.cc>
Precedence: bulk

Am 2021-05-31 10:52, schrieb Tudor.Ambarus@microchip.com:
> On 5/21/21 10:40 PM, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> SPI NOR flashes will just ignore program commands if the OTP region is
>> locked. Thus, a user might not notice that the intended write didn't 
>> end
>> up in the flash. Return -EROFS to the user in this case. From what I 
>> can
>> tell, chips/cfi_cmdset_0001.c also return this error code.
>> 
>> One could optimize spi_nor_mtd_otp_range_is_locked() to read the 
>> status
>> register only once and not for every OTP region, but for that we would
>> need some more invasive changes. Given that this is
>> one-time-programmable memory and the normal access mode is reading, we
>> just live with the small overhead.
> 
> :)
> 
> Shouldn't we change
> struct spi_nor_otp_ops {
> 	...
>         int (*lock)(struct spi_nor *nor, unsigned int region);
>         int (*is_locked)(struct spi_nor *nor, unsigned int region);
> };
> 
> to:
> struct spi_nor_otp_ops {
> 	...
>         int (*lock)(struct spi_nor *nor, loff_t addr, size_t len);
> 
>         int (*is_locked)(struct spi_nor *nor, loff_t addr, size_t len);
> };
> 
> instead?

I had that, but then
  (1) it doesn't fit the hardware (the one's I know of) and the function
      itself would need to convert to the given range
  (2) each lock()/is_locked() would need to implement the "if at least
      one region is locked everything is locked", which might lead to
      different implementations.
  (3) in what address space is addr and len? I'd presume the one of the
      device (so is orthogonal to read()/write()). So if you get lets
      say addr=0x1000 len=512, you'd need to convert that into region
      0 and 1. Thus you'd have this mapping cluttered over all functions.
      And additionally, you'd first need to convert the mtd offsets
      addr=0 len=512 to addr=0x1000 and len=512.

-michael