Re: [PATCH v4] mmc: core: allow detection of locked cards

[Daniel Kucera <linux-mmc@danman.eu>]

Hi Ulf,

could you please reconsider this?
Or give some tips how to make this acceptable for merging?

Thank you,
Daniel.

On 2024-06-21 09:16, Avri Altman wrote:
>> On 2024-06-20 16:32, Ulf Hansson wrote:
>> > On Thu, 20 Jun 2024 at 14:59, Daniel Kucera <linux-mmc@danman.eu>
>> > wrote:
>> >>
>> >> On 2024-06-20 14:38, Ulf Hansson wrote:
>> >> > On Thu, 6 Jun 2024 at 15:12, <linux-mmc@danman.eu> wrote:
>> >> >>
>> >> >> From: Daniel Kucera <linux-mmc@danman.eu>
>> >> >>
>> >> >> Locked SD card will not reply to SEND_SCR or SD_STATUS commands so
>> >> >> it was failing to initialize previously. When skipped, the card
>> >> >> will get initialized and CMD42 can be sent using ioctl to unlock
>> >> >> the card or remove password protection.
>> >> >> For eMMC, this is not necessary because all initialization
>> >> >> commands are allowed in locked state.
>> >> >> Until unlocked, all read/write calls will timeout.
>> >> >
>> >> > Skipping the commands above, only means the card gets partially
>> >> > initialized.
>> >>
>> >> Correct, but it's an improvement in comparison to current state.
>> >
>> > Not sure I agree with that, sorry.
>> 
>> Are you saying that that being able to work with locked card is not an
>> improvement in comparison to not being able to? Or did I misunderstand
>> that?
>> 
>> >
>> >>
>> >> > Leaving a card in that state seems fragile.
>> >>
>> >> Fragile in what sense? Nothing can happen to the card as it is locked.
>> >
>> > We may end up having a card half-way initialized that we can't really
>> > communicate with in a stable manner. From a system point of view, I
>> > would be worried.
> Actually, it's what the spec expects - the CARD_IS_LOCKED is carried
> in CMD7 response.
> Then the card responds to class 0, class 7, and ACMD41.
> 
>> 
>> It's not half-way initialized, it's initialized correctly, it's just 
>> not using the full
>> potential of the card (higher speeds, etc.).
>> 
>> The situation would be the same as it is currently with emmc. Locked 
>> emmc
>> gets initialized correctly but reads/writes time-out. What is wrong in 
>> having
>> the same result for both sd and emmc?
>> 
>> >
>> > I would rather just power off the card if initialization fails and
>> > remove its corresponding device from the system.
>> >
>> >>
>> >> > What will
>> >> > happen to upper block layers and filesystems when trying to access it?
>> >>
>> >> Everything will simply time-out.
>> >
>> > Yes, but it's uncertain what that could lead to?
>> >
>> > What will happen with power consumption and power management
>> support,
>> > for example.
>> 
>> Okay, this is a valid concern. Would it be acceptable if we just 
>> enabled this
>> "feature" with a module parameter, something like 
>> "sd_initialize_locked=1"
>> with default 0?
>> 
>> >
>> >>
>> >> >
>> >> > Several years ago Al Cooper made an attempt [1] to manage the
>> >> > unlocking of the card during initialization, by finding the
>> >> > password through the "keys" subsystem. The series didn't really
>> >> > make it to the upstream kernel, but overall the approach seemed to make
>> sense to me.
>> >> > It should allow us to complete the initialization of the card
>> >> > inside the kernel and prevent unnecessary complexity for userspace to
>> manage.
>> >>
>> >> Yes, he did a great work but obviously no-one got too excited to
>> >> review/merge his work. His solution was complex.
>> >
>> > It's quite some amount of code. On the other hand, it seemed quite
>> > straight forward, not that complex to me.
>> 
>> It doesn't solve the situation when you don't know the password and 
>> you just
>> want to erase the card and continue using in. The (un)locking doesn't 
>> belong
>> to kernel IMO, if it can be implemented in user-space, it should and 
>> it is more
>> flexible that way.
> I also see little value in handling the unlocking process in the 
> kernel.
> I find the proposed approach, e.g.  co-exists with its sibling
> mmc-utils patch, straight forward and simpler.
> 
> Thanks,
> Avri
>> 
>> >
>> >>
>> >> Mine targets the smallest change possible to make it work at least.
>> >> I wanted to avoid a solution that would be hard to test, review and
>> >> maintain.
>> >> Especially when there is only a small interest in the feature.
>> >>
>> >> > Perhaps you can have a closer look at that series and see if it's
>> >> > possible to update it?
>> >>
>> >> I don't think I have the skills to revive his work.
>> >
>> > I see, maybe we should ping Al and other interested folkz to see if
>> > there still is some interest to move this forward?
>> 
>> Okay, Al, are you interested?
>> 
>> >
>> > [...]
>> >
>> > Kind regards
>> > Uffe
>> 
>> BR
>> Daniel
>>