linux-kernel - RE: [PATCH 0/4] Add support SDUC for Realtek card readers

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] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <DM6PR04MB657501EF884BAAC24446E8A1FCB42@DM6PR04MB6575.namprd04.prod.outlook.com>
Date: Fri, 26 Jul 2024 06:13:44 +0000
From: Avri Altman <Avri.Altman@....com>
To: Ricky Wu <ricky_wu@...ltek.com>, "ulf.hansson@...aro.org"
	<ulf.hansson@...aro.org>, "linux-mmc@...r.kernel.org"
	<linux-mmc@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "ricardo@...liere.net"
	<ricardo@...liere.net>, "gregkh@...uxfoundation.org"
	<gregkh@...uxfoundation.org>, "arnd@...db.de" <arnd@...db.de>
Subject: RE: [PATCH 0/4] Add support SDUC for Realtek card readers

Hi,
> Summary:
> this patch is for mmc to support SDUC(Secure Digital Ultra Capacity)
> and add Realtek Cardreaders to support it
> 
> About SDUC:
> SDUC is defined in SD7.0 spec, it support the max capacity is 128TB[1]
> 
> Roughly implemented functions by this patch:
> 1.Mutual identification of SDUC in ACMD41 initialization (5.2.1[2])
> 2.SDUC card user area capacity calculation (5.2.2[2])
> 3.Memory read/write commands sequence (5.2.3[2])
> 4.Erase Commands Sequence (5.2.4[2])
> 
> patches:
> patch#1: Defined functions and modified some type of parameter
> patch#2: For mmc to support SDUC
> patch#3: Add Realtek sdmmc host to support SDUC
> patch#4: Add Realtek card readers to support SDUC
I think that the way you organized your code makes it very hard to review.
I would like to propose rearranging your code in a more readable form.
E.g. something like:

1) mmc: sd: SDUC Support Recognition: 
Explain how ACMD21 was extended to support the host-card handshake during initialization.
Also, make it clear, e.g. in your commit log that if a SDUC card is inserted to a
non-supporting host, it will never respond to this ACMD21 until eventually,
the host will timed out and give up.

2) mmc: sd: Add SD CSD version 3.0
Add here the require changes in core/bus.c, core/card.h, and csd ver 3 in mmc_decode_csd().

3) mmc: sd: Add Extension memory addressing
Add in core/sd_ops.c mmc_send_ext_addr() so you'll be able to call it in open-ended rw, erase, etc.
Here it's a good place to explain the general idea of the new CMD22.

4) mmc: core: Add open-ended Ext memory addressing
Call mmc_send_ext_addr() for open-ended rw

5) mmc: host: Always use manual-cmd23 in SDUC
6) mmc: core: Add close-ended Ext memory addressing
7) mmc: host: Add close-ended Ext memory addressing
Those 3 patches will facilitate close-ended rw.
It should be IMO or any SDHCI and not just for RealTk's.
Once the driver is in place, SDUC support doesn't require specific hw characteristics from the host controller.

8) mmc: core: Add Ext memory addressing for erase
9) mmc: core: Allow mmc erase to carry large addresses
Make the require changes to support erase

10) mmc: core: Adjust ACMD22 to SDUC
Make the adjustments in ACMD22.

I also think that there are some specific issues with your implementation,
But it would be easier to discuss those once the patches are in a more readable form.

Thanks,
Avri


> 
> Reference:
> [1] SD Specifications Part 1 Physical Layer Specification Version 7.00
> [2] SD Specifications SDUC Host Implementation Guideline Version 1.00
> 
> Ricky Wu(4):
>  mmc: core: some definitions and type modifications for SDUC
>  mmc: core: add SDUC init rw erase flow to mmc
>  mmc: rtsx: make Realtek sdmmc to support SDUC
>  misc: rtsx: add Realtek card readers to support SDUC
> 
>  drivers/misc/cardreader/rts5227.c |  1 +
>  drivers/misc/cardreader/rts5228.c |  1 +
>  drivers/misc/cardreader/rts5249.c |  1 +
>  drivers/misc/cardreader/rts5260.c |  1 +
>  drivers/misc/cardreader/rts5261.c |  1 +
>  drivers/misc/cardreader/rts5264.c |  2 +-
>  drivers/mmc/core/block.c          | 13 +++++++--
>  drivers/mmc/core/bus.c            |  4 ++-
>  drivers/mmc/core/card.h           |  3 ++
>  drivers/mmc/core/core.c           | 38 +++++++++++++++++--------
>  drivers/mmc/core/core.h           |  6 ++--
>  drivers/mmc/core/host.h           |  5 ++++
>  drivers/mmc/core/queue.h          |  1 +
>  drivers/mmc/core/sd.c             | 47 +++++++++++++++++++++++++++++++
>  drivers/mmc/host/rtsx_pci_sdmmc.c |  6 ++++
>  include/linux/mmc/card.h          |  2 +-
>  include/linux/mmc/core.h          |  1 +
>  include/linux/mmc/host.h          |  1 +
>  include/linux/mmc/sd.h            |  5 ++++
>  include/linux/rtsx_pci.h          |  1 +
>  20 files changed, 121 insertions(+), 19 deletions(-)
> 
> 
> --
> 2.25.1