diff mbox series

mmc: inline the first mmc_scan() on mmc_start_host()

Message ID 20230329202148.71107-1-dennis@kernel.org
State New
Headers show
Series mmc: inline the first mmc_scan() on mmc_start_host() | expand

Commit Message

Dennis Zhou March 29, 2023, 8:21 p.m. UTC 
When using dm-verity with a data partition on an emmc device, dm-verity
races with the discovery of attached emmc devices. This is because mmc's
probing code sets up the host data structure then a work item is
scheduled to do discovery afterwards. To prevent this race on init,
let's inline the first call to detection, __mm_scan(), and let
subsequent detect calls be handled via the workqueue.

Signed-off-by: Dennis Zhou <dennis@kernel.org>
---
 drivers/mmc/core/core.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Comments

kernel test robot March 29, 2023, 11:36 p.m. UTC | #1 
Hi Dennis,

I love your patch! Perhaps something to improve:

[auto build test WARNING on ulf-hansson-mmc-mirror/next]
[also build test WARNING on linus/master v6.3-rc4 next-20230329]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Dennis-Zhou/mmc-inline-the-first-mmc_scan-on-mmc_start_host/20230330-042213
base:   https://git.linaro.org/people/ulf.hansson/mmc-mirror.git next
patch link:    https://lore.kernel.org/r/20230329202148.71107-1-dennis%40kernel.org
patch subject: [PATCH] mmc: inline the first mmc_scan() on mmc_start_host()
config: hexagon-randconfig-r045-20230329 (https://download.01.org/0day-ci/archive/20230330/202303300728.pAr0H6ZJ-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/d2de7314d2198df0c7a452546af0c15799b2d864
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Dennis-Zhou/mmc-inline-the-first-mmc_scan-on-mmc_start_host/20230330-042213
        git checkout d2de7314d2198df0c7a452546af0c15799b2d864
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/mmc/core/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303300728.pAr0H6ZJ-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/mmc/core/core.c:12:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from drivers/mmc/core/core.c:12:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from drivers/mmc/core/core.c:12:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
>> drivers/mmc/core/core.c:2202:6: warning: no previous prototype for function '__mmc_rescan' [-Wmissing-prototypes]
   void __mmc_rescan(struct mmc_host *host)
        ^
   drivers/mmc/core/core.c:2202:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void __mmc_rescan(struct mmc_host *host)
   ^
   static 
   7 warnings generated.


vim +/__mmc_rescan +2202 drivers/mmc/core/core.c

  2201	
> 2202	void __mmc_rescan(struct mmc_host *host)
  2203	{
  2204		int i;
  2205	
  2206		if (host->rescan_disable)
  2207			return;
  2208	
  2209		/* If there is a non-removable card registered, only scan once */
  2210		if (!mmc_card_is_removable(host) && host->rescan_entered)
  2211			return;
  2212		host->rescan_entered = 1;
  2213	
  2214		if (host->trigger_card_event && host->ops->card_event) {
  2215			mmc_claim_host(host);
  2216			host->ops->card_event(host);
  2217			mmc_release_host(host);
  2218			host->trigger_card_event = false;
  2219		}
  2220	
  2221		/* Verify a registered card to be functional, else remove it. */
  2222		if (host->bus_ops)
  2223			host->bus_ops->detect(host);
  2224	
  2225		host->detect_change = 0;
  2226	
  2227		/* if there still is a card present, stop here */
  2228		if (host->bus_ops != NULL)
  2229			goto out;
  2230	
  2231		mmc_claim_host(host);
  2232		if (mmc_card_is_removable(host) && host->ops->get_cd &&
  2233				host->ops->get_cd(host) == 0) {
  2234			mmc_power_off(host);
  2235			mmc_release_host(host);
  2236			goto out;
  2237		}
  2238	
  2239		/* If an SD express card is present, then leave it as is. */
  2240		if (mmc_card_sd_express(host)) {
  2241			mmc_release_host(host);
  2242			goto out;
  2243		}
  2244	
  2245		for (i = 0; i < ARRAY_SIZE(freqs); i++) {
  2246			unsigned int freq = freqs[i];
  2247			if (freq > host->f_max) {
  2248				if (i + 1 < ARRAY_SIZE(freqs))
  2249					continue;
  2250				freq = host->f_max;
  2251			}
  2252			if (!mmc_rescan_try_freq(host, max(freq, host->f_min)))
  2253				break;
  2254			if (freqs[i] <= host->f_min)
  2255				break;
  2256		}
  2257	
  2258		/*
  2259		 * Ignore the command timeout errors observed during
  2260		 * the card init as those are excepted.
  2261		 */
  2262		host->err_stats[MMC_ERR_CMD_TIMEOUT] = 0;
  2263		mmc_release_host(host);
  2264	
  2265	 out:
  2266		if (host->caps & MMC_CAP_NEEDS_POLL)
  2267			mmc_schedule_delayed_work(&host->detect, HZ);
  2268	}
  2269
Ulf Hansson June 13, 2023, 2:25 p.m. UTC | #2 
On Wed, 29 Mar 2023 at 22:21, Dennis Zhou <dennis@kernel.org> wrote:
>
> When using dm-verity with a data partition on an emmc device, dm-verity
> races with the discovery of attached emmc devices. This is because mmc's
> probing code sets up the host data structure then a work item is
> scheduled to do discovery afterwards. To prevent this race on init,
> let's inline the first call to detection, __mm_scan(), and let
> subsequent detect calls be handled via the workqueue.
>
> Signed-off-by: Dennis Zhou <dennis@kernel.org>

Along with the patch for the mmci driver, this one applied too, for
next, thanks!

Note also that I took the liberty to clarify the commit message a bit.

Moreover, if we want this to be applied for stable kernels, we need to
manage that separately, as then the mmci patch is needed too. Please
ping if you need some pointers in regards to this.

Kind regards
Uffe

> ---
>  drivers/mmc/core/core.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 368f10405e13..c0fdc438c882 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2185,10 +2185,8 @@ int mmc_card_alternative_gpt_sector(struct mmc_card *card, sector_t *gpt_sector)
>  }
>  EXPORT_SYMBOL(mmc_card_alternative_gpt_sector);
>
> -void mmc_rescan(struct work_struct *work)
> +void __mmc_rescan(struct mmc_host *host)
>  {
> -       struct mmc_host *host =
> -               container_of(work, struct mmc_host, detect.work);
>         int i;
>
>         if (host->rescan_disable)
> @@ -2249,6 +2247,14 @@ void mmc_rescan(struct work_struct *work)
>                 mmc_schedule_delayed_work(&host->detect, HZ);
>  }
>
> +void mmc_rescan(struct work_struct *work)
> +{
> +       struct mmc_host *host =
> +               container_of(work, struct mmc_host, detect.work);
> +
> +       __mmc_rescan(host);
> +}
> +
>  void mmc_start_host(struct mmc_host *host)
>  {
>         host->f_init = max(min(freqs[0], host->f_max), host->f_min);
> @@ -2261,7 +2267,8 @@ void mmc_start_host(struct mmc_host *host)
>         }
>
>         mmc_gpiod_request_cd_irq(host);
> -       _mmc_detect_change(host, 0, false);
> +       host->detect_change = 1;
> +       __mmc_rescan(host);
>  }
>
>  void __mmc_stop_host(struct mmc_host *host)
> --
> 2.40.0
>
Dennis Zhou June 15, 2023, 4:06 p.m. UTC | #3 
On Tue, Jun 13, 2023 at 04:25:11PM +0200, Ulf Hansson wrote:
> On Wed, 29 Mar 2023 at 22:21, Dennis Zhou <dennis@kernel.org> wrote:
> >
> > When using dm-verity with a data partition on an emmc device, dm-verity
> > races with the discovery of attached emmc devices. This is because mmc's
> > probing code sets up the host data structure then a work item is
> > scheduled to do discovery afterwards. To prevent this race on init,
> > let's inline the first call to detection, __mm_scan(), and let
> > subsequent detect calls be handled via the workqueue.
> >
> > Signed-off-by: Dennis Zhou <dennis@kernel.org>
> 
> Along with the patch for the mmci driver, this one applied too, for
> next, thanks!
> 

Thank you Ulf! I'm good with this just being applied to for-next.

Thanks,
Dennis

> Note also that I took the liberty to clarify the commit message a bit.
> 
> Moreover, if we want this to be applied for stable kernels, we need to
> manage that separately, as then the mmci patch is needed too. Please
> ping if you need some pointers in regards to this.
> 
> Kind regards
> Uffe
> 
> > ---
> >  drivers/mmc/core/core.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index 368f10405e13..c0fdc438c882 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -2185,10 +2185,8 @@ int mmc_card_alternative_gpt_sector(struct mmc_card *card, sector_t *gpt_sector)
> >  }
> >  EXPORT_SYMBOL(mmc_card_alternative_gpt_sector);
> >
> > -void mmc_rescan(struct work_struct *work)
> > +void __mmc_rescan(struct mmc_host *host)
> >  {
> > -       struct mmc_host *host =
> > -               container_of(work, struct mmc_host, detect.work);
> >         int i;
> >
> >         if (host->rescan_disable)
> > @@ -2249,6 +2247,14 @@ void mmc_rescan(struct work_struct *work)
> >                 mmc_schedule_delayed_work(&host->detect, HZ);
> >  }
> >
> > +void mmc_rescan(struct work_struct *work)
> > +{
> > +       struct mmc_host *host =
> > +               container_of(work, struct mmc_host, detect.work);
> > +
> > +       __mmc_rescan(host);
> > +}
> > +
> >  void mmc_start_host(struct mmc_host *host)
> >  {
> >         host->f_init = max(min(freqs[0], host->f_max), host->f_min);
> > @@ -2261,7 +2267,8 @@ void mmc_start_host(struct mmc_host *host)
> >         }
> >
> >         mmc_gpiod_request_cd_irq(host);
> > -       _mmc_detect_change(host, 0, false);
> > +       host->detect_change = 1;
> > +       __mmc_rescan(host);
> >  }
> >
> >  void __mmc_stop_host(struct mmc_host *host)
> > --
> > 2.40.0
> >
diff mbox series

Patch

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 368f10405e13..c0fdc438c882 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2185,10 +2185,8 @@  int mmc_card_alternative_gpt_sector(struct mmc_card *card, sector_t *gpt_sector)
 }
 EXPORT_SYMBOL(mmc_card_alternative_gpt_sector);
 
-void mmc_rescan(struct work_struct *work)
+void __mmc_rescan(struct mmc_host *host)
 {
-	struct mmc_host *host =
-		container_of(work, struct mmc_host, detect.work);
 	int i;
 
 	if (host->rescan_disable)
@@ -2249,6 +2247,14 @@  void mmc_rescan(struct work_struct *work)
 		mmc_schedule_delayed_work(&host->detect, HZ);
 }
 
+void mmc_rescan(struct work_struct *work)
+{
+	struct mmc_host *host =
+		container_of(work, struct mmc_host, detect.work);
+
+	__mmc_rescan(host);
+}
+
 void mmc_start_host(struct mmc_host *host)
 {
 	host->f_init = max(min(freqs[0], host->f_max), host->f_min);
@@ -2261,7 +2267,8 @@  void mmc_start_host(struct mmc_host *host)
 	}
 
 	mmc_gpiod_request_cd_irq(host);
-	_mmc_detect_change(host, 0, false);
+	host->detect_change = 1;
+	__mmc_rescan(host);
 }
 
 void __mmc_stop_host(struct mmc_host *host)