Received-SPF: pass (google.com: domain of linux-kernel+bounces-93597-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: "Jorge Ramirez-Ortiz, Foundries" <jorge@foundries.io>
Date: Wed, 6 Mar 2024 10:05:40 +0100
To: Dominique Martinet <asmadeus@codewreck.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Jorge Ramirez-Ortiz <jorge@foundries.io>, linux-mmc@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Dominique Martinet <dominique.martinet@atmark-techno.com>,
	stable@vger.kernel.org
Subject: Re: [PATCH] mmc: part_switch: fixes switch on gp3 partition
Message-ID: <Zegx5PCtg6hs8zyp@trax>
References: <20240306-mmc-partswitch-v1-1-bf116985d950@codewreck.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240306-mmc-partswitch-v1-1-bf116985d950@codewreck.org>

On 06/03/24 10:44:38, Dominique Martinet wrote:
> From: Dominique Martinet <dominique.martinet@atmark-techno.com>
>
> Commit e7794c14fd73 ("mmc: rpmb: fixes pause retune on all RPMB
> partitions.") added a mask check for 'part_type', but the mask used was
> wrong leading to the code intended for rpmb also being executed for GP3.
>
> On some MMCs (but not all) this would make gp3 partition inaccessible:
> armadillo:~# head -c 1 < /dev/mmcblk2gp3
> head: standard input: I/O error
> armadillo:~# dmesg -c
> [  422.976583] mmc2: running CQE recovery
> [  423.058182] mmc2: running CQE recovery
> [  423.137607] mmc2: running CQE recovery
> [  423.137802] blk_update_request: I/O error, dev mmcblk2gp3, sector 0 op 0x0:(READ) flags 0x80700 phys_seg 4 prio class 0
> [  423.237125] mmc2: running CQE recovery
> [  423.318206] mmc2: running CQE recovery
> [  423.397680] mmc2: running CQE recovery
> [  423.397837] blk_update_request: I/O error, dev mmcblk2gp3, sector 0 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0
> [  423.408287] Buffer I/O error on dev mmcblk2gp3, logical block 0, async page read
>
> the part_type values of interest here are defined as follow:
> main  0
> boot0 1
> boot1 2
> rpmb  3
> gp0   4
> gp1   5
> gp2   6
> gp3   7

right, the patch I originally sent didn't consider anything after GP0 as per
the definitions below.

#define EXT_CSD_PART_CONFIG_ACC_MASK	(0x7)
#define EXT_CSD_PART_CONFIG_ACC_BOOT0	(0x1)
#define EXT_CSD_PART_CONFIG_ACC_RPMB	(0x3)
#define EXT_CSD_PART_CONFIG_ACC_GP0	(0x4)

That looked strange as there should be support for 4 GP but this code
kind of convinced me of the opposite.

	if (idata->rpmb) {
		/* Support multiple RPMB partitions */
		target_part = idata->rpmb->part_index;
		target_part |= EXT_CSD_PART_CONFIG_ACC_RPMB;
	}

So if we apply the fix that you propose, how are multiple RPMB
partitions (ie, 4) going to be identified as RPMB? Unless there can't be
more than 3?

But sure, your patch makes sense to me.

>
> so mask with EXT_CSD_PART_CONFIG_ACC_MASK (7) to correctly identify rpmb
>
> Fixes: e7794c14fd73 ("mmc: rpmb: fixes pause retune on all RPMB partitions.")
> Cc: stable@vger.kernel.org
> Cc: Jorge Ramirez-Ortiz <jorge@foundries.io>
> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
> ---
> A couple of notes:
> - this doesn't fail on all eMMCs, I can still access gp3 on some models
>   but it seems to fail reliably with micron's "G1M15L"
> - I've encountered this on the 5.10 backport (in 5.10.208), so that'll
>   need to be backported everywhere the fix was taken...
>
> Thanks!
> ---
>  drivers/mmc/core/block.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 32d49100dff5..86efa6084696 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -874,10 +874,11 @@ static const struct block_device_operations mmc_bdops = {
>  static int mmc_blk_part_switch_pre(struct mmc_card *card,
>  				   unsigned int part_type)
>  {
> -	const unsigned int mask = EXT_CSD_PART_CONFIG_ACC_RPMB;
> +	const unsigned int mask = EXT_CSD_PART_CONFIG_ACC_MASK;
> +	const unsigned int rpmb = EXT_CSD_PART_CONFIG_ACC_RPMB;
>  	int ret = 0;
>
> -	if ((part_type & mask) == mask) {
> +	if ((part_type & mask) == rpmb) {
>  		if (card->ext_csd.cmdq_en) {
>  			ret = mmc_cmdq_disable(card);
>  			if (ret)
> @@ -892,10 +893,11 @@ static int mmc_blk_part_switch_pre(struct mmc_card *card,
>  static int mmc_blk_part_switch_post(struct mmc_card *card,
>  				    unsigned int part_type)
>  {
> -	const unsigned int mask = EXT_CSD_PART_CONFIG_ACC_RPMB;
> +	const unsigned int mask = EXT_CSD_PART_CONFIG_ACC_MASK;
> +	const unsigned int rpmb = EXT_CSD_PART_CONFIG_ACC_RPMB;
>  	int ret = 0;
>
> -	if ((part_type & mask) == mask) {
> +	if ((part_type & mask) == rpmb) {
>  		mmc_retune_unpause(card->host);
>  		if (card->reenable_cmdq && !card->ext_csd.cmdq_en)
>  			ret = mmc_cmdq_enable(card);
>
> ---
> base-commit: 5847c9777c303a792202c609bd761dceb60f4eed
> change-id: 20240306-mmc-partswitch-c3a50b5084ae
>
> Best regards,
> --
> Dominique Martinet | Asmadeus
>