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;
User-Agent: Cyrus-JMAP/3.5.0-alpha0-93-gef6c4048e6-fm-20210128.002-gef6c4048
Mime-Version: 1.0
Message-Id: <b21034dc-f511-448f-9c1d-68013c640608@www.fastmail.com>
In-Reply-To: <20210104122853.18428-1-git@johnthomson.fastmail.com.au>
References: <20210104122853.18428-1-git@johnthomson.fastmail.com.au>
Date:   Tue, 02 Feb 2021 20:08:43 +0000
From:   "John Thomson" <git@johnthomson.fastmail.com.au>
To:     "Miquel Raynal" <miquel.raynal@bootlin.com>,
        "Richard Weinberger" <richard@nod.at>,
        "Vignesh Raghavendra" <vigneshr@ti.com>,
        "Tudor Ambarus" <tudor.ambarus@microchip.com>
Cc:     linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: =?UTF-8?Q?Re:_[RFC_PATCH]_mtd:_spi-nor:_write_support_for_minor_aligned_?=
 =?UTF-8?Q?partitions?=
Content-Type: text/plain
Precedence: bulk

On Mon, 4 Jan 2021, at 12:28, John Thomson wrote:
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -38,10 +38,11 @@ static struct mtd_info *allocate_partition(struct 
> mtd_info *parent,
>  	struct mtd_info *master = mtd_get_master(parent);
>  	int wr_alignment = (parent->flags & MTD_NO_ERASE) ?
>  			   master->writesize : master->erasesize;
> +	int wr_alignment_minor;

int wr_alignment_minor = 0;

> +	if (!(child->flags & MTD_NO_ERASE)) {
> 	wr_alignment = child->erasesize;
> +	if (child->erasesize_minor)

if (child->erasesize_minor && IS_ENABLED(CONFIG_MTD_SPI_NOR_USE_VARIABLE_ERASE)) {
Config test wrap wr_alignment_minor being set,
so that a partition on a minor boundary is only set writeable if the non-uniform erase path can be used.

> +		wr_alignment_minor = child->erasesize_minor;
> +	}

> +		if (wr_alignment_minor) {

smatch picked up a tested uninitialized symbol 'wr_alignment_minor' here,
initialise as 0.
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

> +		if ((!wr_alignment_minor) || (wr_alignment_minor && remainder_minor != 0)) {

If it is safe to boolean test the ints and u32, I should use this consistently?
if ((!wr_alignment_minor) || (wr_alignment_minor && remainder_minor)) {
Or is it clearer to use the math tests?
if ((wr_alignment_minor == 0) || (wr_alignment_minor > 0 && remainder_minor > 0)) {

> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1225,7 +1225,11 @@ static u8 spi_nor_convert_3to4_erase(u8 opcode)
>  
>  static bool spi_nor_has_uniform_erase(const struct spi_nor *nor)
>  {
> +#ifdef CONFIG_MTD_SPI_NOR_USE_VARIABLE_ERASE

if (IS_ENABLED(CONFIG_MTD_SPI_NOR_USE_VARIABLE_ERASE)) {
and the closing brace better than the #ifdef here?

> +	return false;
> +#else
>  	return !!nor->params->erase_map.uniform_erase_type;
> +#endif
>  }
>  
>  static void spi_nor_set_4byte_opcodes(struct spi_nor *nor)


Otherwise, is this approach valid, or is there a better method I can use?

Cheers,
-- 
  John Thomson