From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933048AbcFBPBs (ORCPT ); Thu, 2 Jun 2016 11:01:48 -0400 Received: from mail-wm0-f50.google.com ([74.125.82.50]:36536 "EHLO mail-wm0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932943AbcFBPBq convert rfc822-to-8bit (ORCPT ); Thu, 2 Jun 2016 11:01:46 -0400 MIME-Version: 1.0 In-Reply-To: References: <1464505484-3661-1-git-send-email-wens@csie.org> <1464505484-3661-2-git-send-email-wens@csie.org> Date: Thu, 2 Jun 2016 17:01:44 +0200 Message-ID: Subject: Re: [PATCH 1/3] mmc: fix mmc mode selection for HS-DDR and higher From: Ulf Hansson To: Krzysztof Kozlowski Cc: Chen-Yu Tsai , Arnd Bergmann , Maxime Ripard , Hans de Goede , linux-mmc , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , Linus Torvalds Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2 June 2016 at 11:35, Krzysztof Kozlowski wrote: > On Thu, Jun 2, 2016 at 10:31 AM, Ulf Hansson wrote: >> + Linus >> >> On 29 May 2016 at 09:04, Chen-Yu Tsai wrote: >>> When IS_ERR_VALUE was removed from the mmc core code, it was replaced >>> with a simple not-zero check. This does not work, as the value checked >>> is the return value for mmc_select_bus_width, which returns the set >>> bit width on success. This made eMMC modes higher than HS-DDR unusable. >>> >>> Fix this by checking for a positive return value instead. >>> >>> Fixes: 287980e49ffc ("remove lots of IS_ERR_VALUE abuses") >>> Cc: Arnd Bergmann >>> Signed-off-by: Chen-Yu Tsai >>> --- >>> drivers/mmc/core/mmc.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c >>> index c984321d1881..aafb73d080ca 100644 >>> --- a/drivers/mmc/core/mmc.c >>> +++ b/drivers/mmc/core/mmc.c >>> @@ -1276,7 +1276,7 @@ static int mmc_select_hs200(struct mmc_card *card) >>> * switch to HS200 mode if bus width is set successfully. >>> */ >>> err = mmc_select_bus_width(card); >>> - if (!err) { >>> + if (err > 0) { >>> val = EXT_CSD_TIMING_HS200 | >>> card->drive_strength << EXT_CSD_DRV_STR_SHIFT; >>> err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, >>> @@ -1583,7 +1583,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr, >>> } else if (mmc_card_hs(card)) { >>> /* Select the desired bus width optionally */ >>> err = mmc_select_bus_width(card); >>> - if (!err) { >>> + if (err > 0) { >> >> As pointed out in the review by Björn, to restore the old behaviour we >> should check for "err >= 0". >> No need to send a new patch, I can amend the current version. >> >>> err = mmc_select_hs_ddr(card); >>> if (err) >>> goto free_card; >>> -- >>> 2.8.1 >>> >> >> Finally, I am a little concerned about the commit 287980e49ffc >> ("remove lots of IS_ERR_VALUE abuses") which introduced this >> regression. >> >> Surprisingly the IS_ERR_VALUE():s aren't being replaced by equivalent >> checks, so perhaps there a more regressions. Moreover, I wonder why I >> wasn't being on cc/to list when this patch was submitted a few days >> ago, perhaps my review could prevented the regression from even >> happen. >> >> Anyway, let's fix this now! I will pick up $subject patch as fix asap... >> >> and Arnd, can you please double-check that the commit 287980e49ffc >> doesn’t seems to regress anything else!? > > If only the 287980e49ffc could sit in linux-next for few days before > reaching v4.7-rc1... Could you please pick up the fix soon? Maybe > directly by Linus? The fix has already been applied and published through my mmc tree. I am waiting for reports from kernelci, assuming those will be okay, I will send a PR tomorrow so it should reach rc2. Kind regards Uffe > > Best regards, > Krzysztof From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ulf Hansson Subject: Re: [PATCH 1/3] mmc: fix mmc mode selection for HS-DDR and higher Date: Thu, 2 Jun 2016 17:01:44 +0200 Message-ID: References: <1464505484-3661-1-git-send-email-wens@csie.org> <1464505484-3661-2-git-send-email-wens@csie.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from mail-wm0-f45.google.com ([74.125.82.45]:38157 "EHLO mail-wm0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933033AbcFBPBq convert rfc822-to-8bit (ORCPT ); Thu, 2 Jun 2016 11:01:46 -0400 Received: by mail-wm0-f45.google.com with SMTP id a20so71024060wma.1 for ; Thu, 02 Jun 2016 08:01:45 -0700 (PDT) In-Reply-To: Sender: linux-mmc-owner@vger.kernel.org List-Id: linux-mmc@vger.kernel.org To: Krzysztof Kozlowski Cc: Chen-Yu Tsai , Arnd Bergmann , Maxime Ripard , Hans de Goede , linux-mmc , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , Linus Torvalds On 2 June 2016 at 11:35, Krzysztof Kozlowski = wrote: > On Thu, Jun 2, 2016 at 10:31 AM, Ulf Hansson = wrote: >> + Linus >> >> On 29 May 2016 at 09:04, Chen-Yu Tsai wrote: >>> When IS_ERR_VALUE was removed from the mmc core code, it was replac= ed >>> with a simple not-zero check. This does not work, as the value chec= ked >>> is the return value for mmc_select_bus_width, which returns the set >>> bit width on success. This made eMMC modes higher than HS-DDR unusa= ble. >>> >>> Fix this by checking for a positive return value instead. >>> >>> Fixes: 287980e49ffc ("remove lots of IS_ERR_VALUE abuses") >>> Cc: Arnd Bergmann >>> Signed-off-by: Chen-Yu Tsai >>> --- >>> drivers/mmc/core/mmc.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c >>> index c984321d1881..aafb73d080ca 100644 >>> --- a/drivers/mmc/core/mmc.c >>> +++ b/drivers/mmc/core/mmc.c >>> @@ -1276,7 +1276,7 @@ static int mmc_select_hs200(struct mmc_card *= card) >>> * switch to HS200 mode if bus width is set successfully. >>> */ >>> err =3D mmc_select_bus_width(card); >>> - if (!err) { >>> + if (err > 0) { >>> val =3D EXT_CSD_TIMING_HS200 | >>> card->drive_strength << EXT_CSD_DRV_STR_SHIFT= ; >>> err =3D __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, >>> @@ -1583,7 +1583,7 @@ static int mmc_init_card(struct mmc_host *hos= t, u32 ocr, >>> } else if (mmc_card_hs(card)) { >>> /* Select the desired bus width optionally */ >>> err =3D mmc_select_bus_width(card); >>> - if (!err) { >>> + if (err > 0) { >> >> As pointed out in the review by Bj=C3=B6rn, to restore the old behav= iour we >> should check for "err >=3D 0". >> No need to send a new patch, I can amend the current version. >> >>> err =3D mmc_select_hs_ddr(card); >>> if (err) >>> goto free_card; >>> -- >>> 2.8.1 >>> >> >> Finally, I am a little concerned about the commit 287980e49ffc >> ("remove lots of IS_ERR_VALUE abuses") which introduced this >> regression. >> >> Surprisingly the IS_ERR_VALUE():s aren't being replaced by equivalen= t >> checks, so perhaps there a more regressions. Moreover, I wonder why = I >> wasn't being on cc/to list when this patch was submitted a few days >> ago, perhaps my review could prevented the regression from even >> happen. >> >> Anyway, let's fix this now! I will pick up $subject patch as fix asa= p... >> >> and Arnd, can you please double-check that the commit 287980e49ffc >> doesn=E2=80=99t seems to regress anything else!? > > If only the 287980e49ffc could sit in linux-next for few days before > reaching v4.7-rc1... Could you please pick up the fix soon? Maybe > directly by Linus? The fix has already been applied and published through my mmc tree. I am waiting for reports from kernelci, assuming those will be okay, I will send a PR tomorrow so it should reach rc2. Kind regards Uffe > > Best regards, > Krzysztof From mboxrd@z Thu Jan 1 00:00:00 1970 From: ulf.hansson@linaro.org (Ulf Hansson) Date: Thu, 2 Jun 2016 17:01:44 +0200 Subject: [PATCH 1/3] mmc: fix mmc mode selection for HS-DDR and higher In-Reply-To: References: <1464505484-3661-1-git-send-email-wens@csie.org> <1464505484-3661-2-git-send-email-wens@csie.org> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 2 June 2016 at 11:35, Krzysztof Kozlowski wrote: > On Thu, Jun 2, 2016 at 10:31 AM, Ulf Hansson wrote: >> + Linus >> >> On 29 May 2016 at 09:04, Chen-Yu Tsai wrote: >>> When IS_ERR_VALUE was removed from the mmc core code, it was replaced >>> with a simple not-zero check. This does not work, as the value checked >>> is the return value for mmc_select_bus_width, which returns the set >>> bit width on success. This made eMMC modes higher than HS-DDR unusable. >>> >>> Fix this by checking for a positive return value instead. >>> >>> Fixes: 287980e49ffc ("remove lots of IS_ERR_VALUE abuses") >>> Cc: Arnd Bergmann >>> Signed-off-by: Chen-Yu Tsai >>> --- >>> drivers/mmc/core/mmc.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c >>> index c984321d1881..aafb73d080ca 100644 >>> --- a/drivers/mmc/core/mmc.c >>> +++ b/drivers/mmc/core/mmc.c >>> @@ -1276,7 +1276,7 @@ static int mmc_select_hs200(struct mmc_card *card) >>> * switch to HS200 mode if bus width is set successfully. >>> */ >>> err = mmc_select_bus_width(card); >>> - if (!err) { >>> + if (err > 0) { >>> val = EXT_CSD_TIMING_HS200 | >>> card->drive_strength << EXT_CSD_DRV_STR_SHIFT; >>> err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, >>> @@ -1583,7 +1583,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr, >>> } else if (mmc_card_hs(card)) { >>> /* Select the desired bus width optionally */ >>> err = mmc_select_bus_width(card); >>> - if (!err) { >>> + if (err > 0) { >> >> As pointed out in the review by Bj?rn, to restore the old behaviour we >> should check for "err >= 0". >> No need to send a new patch, I can amend the current version. >> >>> err = mmc_select_hs_ddr(card); >>> if (err) >>> goto free_card; >>> -- >>> 2.8.1 >>> >> >> Finally, I am a little concerned about the commit 287980e49ffc >> ("remove lots of IS_ERR_VALUE abuses") which introduced this >> regression. >> >> Surprisingly the IS_ERR_VALUE():s aren't being replaced by equivalent >> checks, so perhaps there a more regressions. Moreover, I wonder why I >> wasn't being on cc/to list when this patch was submitted a few days >> ago, perhaps my review could prevented the regression from even >> happen. >> >> Anyway, let's fix this now! I will pick up $subject patch as fix asap... >> >> and Arnd, can you please double-check that the commit 287980e49ffc >> doesn?t seems to regress anything else!? > > If only the 287980e49ffc could sit in linux-next for few days before > reaching v4.7-rc1... Could you please pick up the fix soon? Maybe > directly by Linus? The fix has already been applied and published through my mmc tree. I am waiting for reports from kernelci, assuming those will be okay, I will send a PR tomorrow so it should reach rc2. Kind regards Uffe > > Best regards, > Krzysztof