linux-kernel - Re: [PATCH V22 02/22] mmc: core: Prepare to support SD UHS-II cards

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAPDyKFpLOdydbOEe4KSmVc_C=umAieR_+Bo--JmVRDtPiX3YuQ@mail.gmail.com>
Date: Wed, 9 Oct 2024 12:37:36 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Kees Bakker <kees@...erbout.nl>
Cc: Victor Shih <victorshihgli@...il.com>, adrian.hunter@...el.com, 
	linux-mmc@...r.kernel.org, linux-kernel@...r.kernel.org, 
	benchuanggli@...il.com, Lucas.Lai@...esyslogic.com.tw, 
	HL.Liu@...esyslogic.com.tw, Greg.tu@...esyslogic.com.tw, dlunev@...omium.org
Subject: Re: [PATCH V22 02/22] mmc: core: Prepare to support SD UHS-II cards

On Tue, 8 Oct 2024 at 21:56, Kees Bakker <kees@...erbout.nl> wrote:
>
> Op 13-09-2024 om 12:28 schreef Victor Shih:
> > From: Ulf Hansson <ulf.hansson@...aro.org>
> >
> > The SD UHS-II interface was introduced to the SD spec v4.00 several years
> > ago. The interface is fundamentally different from an electrical and a
> > protocol point of view, comparing to the legacy SD interface.
> >
> > However, the legacy SD protocol is supported through a specific transport
> > layer (SD-TRAN) defined in the UHS-II addendum of the spec. This allows the
> > SD card to be managed in a very similar way as a legacy SD card, hence a
> > lot of code can be re-used to support these new types of cards through the
> > mmc subsystem.
> >
> > Moreover, an SD card that supports the UHS-II interface shall also be
> > backwards compatible with the legacy SD interface, which allows a UHS-II
> > card to be inserted into a legacy slot. As a matter of fact, this is
> > already supported by mmc subsystem as of today.
> >
> > To prepare to add support for UHS-II, this change puts the basic foundation
> > in the mmc core in place, allowing it to be more easily reviewed before
> > subsequent changes implements the actual support.
> >
> > Basically, the approach here adds a new UHS-II bus_ops type and adds a
> > separate initialization path for the UHS-II card. The intent is to avoid us
> > from sprinkling the legacy initialization path, but also to simplify
> > implementation of the UHS-II specific bits.
> >
> > At this point, there is only one new host ops added to manage the various
> > ios settings needed for UHS-II. Additional host ops that are needed, are
> > being added from subsequent changes.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@...aro.org>
> > ---
> >
> > Updates in V10:
> >   - Drop unnecessary definitions and code.
> >
> > Updates in V9:
> >   - move sd_uhs2_operation definition of PatchV8[05/23] to
> >     PatchV9[02/23] for avoid compilation errors.
> >   - move uhs2_control definition of PatchV8[05/23] to
> >     PatchV9[02/23] for avoid compilation errors.
> >   - move mmc_host flags definition of PatchV8[05/23] to
> >     PatchV9[02/23] for avoid compilation errors.
> >   - move mmc_host flags MMC_UHS2_SUPPORT definition of PatchV8[05/23] to
> >     PatchV9[02/23] for avoid compilation errors.
> >   - move mmc_host flags MMC_UHS2_SD_TRAN definition of PatchV8[05/23] to
> >     PatchV9[02/23] for avoid compilation errors.
> >
> > Updates in V7:
> >   - Drop sd_uhs2_set_ios function.
> >   - Used ->uhs2_control() callback for uhs2_set_ios in sd_uhs2_power_up().
> >   - Used ->uhs2_control() callback for uhs2_set_ios in sd_uhs2_power_off().
> >   - Drop MMC_TIMING_SD_UHS2 in favor of MMC_TIMING_UHS2_SPEED_A.
> >   - Modify sd_uhs2_legacy_init to avoid sd_uhs2_reinit cycle issue.
> >
> > Updates in V4:
> >   - Re-based, updated a comment and removed white-space.
> >   - Moved MMC_VQMMC2_VOLTAGE_180 into a later patch in the series.
> >
> > ---
> >
> >   drivers/mmc/core/Makefile  |   2 +-
> >   drivers/mmc/core/core.c    |  17 ++-
> >   drivers/mmc/core/core.h    |   1 +
> >   drivers/mmc/core/sd_uhs2.c | 292 +++++++++++++++++++++++++++++++++++++
> >   include/linux/mmc/card.h   |   7 +
> >   include/linux/mmc/host.h   |  23 +++
> >   6 files changed, 340 insertions(+), 2 deletions(-)
> >   create mode 100644 drivers/mmc/core/sd_uhs2.c
> >
> > [...]
> > +/*
> > + * Run the enumeration process by sending the enumerate command to the card.
> > + * Note that, we currently support only the point to point connection, which
> > + * means only one card can be attached per host/slot.
> > + */
> > +static int sd_uhs2_enum(struct mmc_host *host, u32 *node_id)
> > +{
> > +     return 0;
> > +}
> > [...]
> > +/*
> > + * Allocate the data structure for the mmc_card and run the UHS-II specific
> > + * initialization sequence.
> > + */
> > +static int sd_uhs2_init_card(struct mmc_host *host)
> > +{
> > +     struct mmc_card *card;
> > +     u32 node_id;
> > +     int err;
> > +
> > +     err = sd_uhs2_dev_init(host);
> > +     if (err)
> > +             return err;
> > +
> > +     err = sd_uhs2_enum(host, &node_id);
> node_id is still uninitialized, see implementation of sd_uhs2_enum above
> > +     if (err)
> > +             return err;
> > +
> > +     card = mmc_alloc_card(host, &sd_type);
> > +     if (IS_ERR(card))
> > +             return PTR_ERR(card);
> > +
> > +     card->uhs2_config.node_id = node_id;
> Using uninitialized node_id

I practise this should not happen as the thought is that
sd_uhs2_enum() should return an error code, unless it can provide the
node_id.

In any case, it's better to initialize node_id at declaration, so I
have amended the patch like that.

[...]

Thanks for reviewing!

Kind regards
Uffe