| 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
| ||
|
Message-ID: <f9b3c046-011c-e5cd-d822-78a7200a6159@gmail.com>
Date: Fri, 7 Sep 2018 22:31:27 +0200
From: Marek Vasut <marek.vasut@...il.com>
To: Tudor Ambarus <tudor.ambarus@...rochip.com>,
cyrille.pitchen@...rochip.com, dwmw2@...radead.org,
computersforpeace@...il.com, boris.brezillon@...tlin.com,
richard@....at
Cc: linux-mtd@...ts.infradead.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
nicolas.ferre@...rochip.com, Cristian.Birsan@...rochip.com
Subject: Re: [RESEND PATCH v2 1/3] mtd: spi-nor: add support to non-uniform
SFDP SPI NOR flash memories
On 09/07/2018 10:51 AM, Tudor Ambarus wrote:
> Thanks Marek,
>
> On 09/03/2018 08:37 PM, Marek Vasut wrote:
>> On 08/27/2018 12:26 PM, Tudor Ambarus wrote:
>> [...]
>>
>>> +/* JEDEC JESD216B Standard imposes erase sizes to be power of 2. */
>>> +static inline u64
>>> +spi_nor_div_by_erase_size(const struct spi_nor_erase_type *erase,
>>> + u64 dividend, u32 *remainder)
>>> +{
>>> + *remainder = (u32)dividend & erase->size_mask;
>>
>> Is the cast really needed ? btw I think there might be a macro doing
>> just this, div_by_ or something in include/ .
>
> The cast is not needed, the AND sets to zero all but the low-order 32bits of
> divided and then we have the implicit cast.
>
> Are you referring to do_div()? I expect the bitwise operations to be faster.
> Bitwise operations are preferred in include/linux/mtd/mtd.h too:
I thought there was some macro to do this bitwise modulo operation
already , so you don't have to implement it here. I don't mean do_div, no.
> static inline uint32_t mtd_div_by_eb(uint64_t sz, struct mtd_info *mtd)
> {
> if (mtd->erasesize_shift)
> return sz >> mtd->erasesize_shift;
> do_div(sz, mtd->erasesize);
> return sz;
> }
>
>>
>>> + return dividend >> erase->size_shift;
>>> +}
>>> +
>>> +static const struct spi_nor_erase_type *
>>> +spi_nor_find_best_erase_type(const struct spi_nor_erase_map *map,
>>> + const struct spi_nor_erase_region *region,
>>> + u64 addr, u32 len)
>>> +{
>>> + const struct spi_nor_erase_type *erase;
>>> + u32 rem;
>>> + int i;
>>> + u8 erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
>>> +
>>> + /*
>>> + * Erase types are ordered by size, with the biggest erase type at
>>> + * index 0.
>>> + */
>>> + for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
>>> + /* Does the erase region support the tested erase type? */
>>> + if (!(erase_mask & BIT(i)))
>>> + continue;
>>> +
>>> + erase = &map->erase_type[i];
>>> +
>>> + /* Don't erase more than what the user has asked for. */
>>> + if (erase->size > len)
>>> + continue;
>>> +
>>> + /* Alignment is not mandatory for overlaid regions */
>>> + if (region->offset & SNOR_OVERLAID_REGION)
>>> + return erase;
>>> +
>>> + spi_nor_div_by_erase_size(erase, addr, &rem);
>>> + if (rem)
>>> + continue;
>>> + else
>>> + return erase;
>>> + }
>>> +
>>> + return NULL;
>>> +}
>>> +
>>> +static struct spi_nor_erase_region *
>>> +spi_nor_region_next(struct spi_nor_erase_region *region)
>>> +{
>>> + if (spi_nor_region_is_last(region))
>>> + return NULL;
>>> + return ++region;
>>
>> region++ ...
>
> It's an array of regions, consecutive in address space, in which walking is done
> incrementally. If the received region is not the last, I want to return the next
> region, so ++region is correct.
return i++ is the same as return ++i;
>> [...]
>>
>>> +static int spi_nor_cmp_erase_type(const void *a, const void *b)
>>> +{
>>> + const struct spi_nor_erase_type *erase1 = a;
>>> + const struct spi_nor_erase_type *erase2 = b;
>>> +
>>> + return erase1->size - erase2->size;
>>
>> What does this function do again ?
>
> It's a compare function, I compare by size the map's Erase Types. I pass a
> pointer to this function in the sort() call. I sort in ascending order, by size,
> all the map's Erase Types when parsing bfpt. I'm doing the sort at init to speed
> up the finding of the best erase command at run-time.
>
> A better name for this function is spi_nor_map_cmp_erase_type(), we compare the
> map's Erase Types by size.
More like a description would be most welcome, in the actual code. And I
really dislike how you fiddle around with void pointers, can't that be
fixed?
>>> +}
>>> +
>>> +static void spi_nor_regions_sort_erase_types(struct spi_nor_erase_map *map)
>>> +{
>>> + struct spi_nor_erase_region *region = map->regions;
>>> + struct spi_nor_erase_type *erase_type = map->erase_type;
>>> + int i;
>>> + u8 region_erase_mask, ordered_erase_mask;
>>> +
>>> + /*
>>> + * Sort each region's Erase Types in ascending order with the smallest
>>> + * Erase Type size starting at BIT(0).
>>> + */
>>> + while (region) {
>>> + region_erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
>>> +
>>> + /*
>>> + * The region's erase mask indicates which erase types are
>>> + * supported from the erase types defined in the map.
>>> + */
>>> + ordered_erase_mask = 0;
>>> + for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++)
>>> + if (erase_type[i].size &&
>>> + region_erase_mask & BIT(erase_type[i].idx))
>>> + ordered_erase_mask |= BIT(i);
>>> +
>>> + /* Overwrite erase mask. */
>>> + region->offset = (region->offset & ~SNOR_ERASE_TYPE_MASK) |
>>> + ordered_erase_mask;
>>> +
>>> + region = spi_nor_region_next(region);
>>> + }
>>> +}
>>> +
>>> +static inline void
>>
>> Drop the inline
>
> Ok.
>
>>
>>> +spi_nor_init_uniform_erase_map(struct spi_nor_erase_map *map,
>>> + u8 erase_mask, u64 flash_size)
>>> +{
>>> + map->uniform_region.offset = SNOR_ERASE_FLAGS_OFFSET(erase_mask, 1, 0,
>>> + 0);
>>> + map->uniform_region.size = flash_size;
>>> + map->regions = &map->uniform_region;
>>> + map->uniform_erase_type = erase_mask;
>>> +}
>>> +
>>
>> [...]
>>
>>> +#define spi_nor_region_is_last(region) (region->offset & SNOR_LAST_REGION)
>>> +
>>> +static inline u64 spi_nor_region_end(const struct spi_nor_erase_region *region)
>>
>> Get rid of the inlines, really.
>
> Agreed.
>
>>
>>> +{
>>> + return (region->offset & ~SNOR_ERASE_FLAGS_MASK) + region->size;
>>> +}
>>> +
>>> +static inline bool spi_nor_has_uniform_erase(const struct spi_nor *nor)
>>> +{
>>> + return !!nor->erase_map.uniform_erase_type;
>>> +}
>>> +
>>> static inline void spi_nor_set_flash_node(struct spi_nor *nor,
>>> struct device_node *np)
>>> {
>>>
>>
>> General question, what happens if the multi-block erase fails mid-way ,
>> is that handled or reported somehow to the user ?
>
> I already implemented your suggestion. I build a list of erase commands to be
> executed once I validate that the erase can be performed.
You can send them, but what happens if it fails mid-way ? Is the user
somehow notified that part of his flash is empty and part is not ?
--
Best regards,
Marek Vasut
Powered by blists - more mailing lists