* [PATCH][MMC] Fix wrong EXT_CSD_REV handling
@ 2007-12-13 7:13 Kyungmin Park
2007-12-13 7:53 ` Pierre Ossman
0 siblings, 1 reply; 6+ messages in thread
From: Kyungmin Park @ 2007-12-13 7:13 UTC (permalink / raw)
To: drzeus-list; +Cc: linux-kernel
It already checked the ext_csd_struct is less than 2,
so it doesn't need to check it.
Current code only accepts the revision 1.2.
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 68c0e3b..7689760 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -217,15 +217,13 @@ static int mmc_read_ext_csd(struct mmc_card *card)
goto out;
}
- if (ext_csd_struct >= 2) {
- card->ext_csd.sectors =
- ext_csd[EXT_CSD_SEC_CNT + 0] << 0 |
- ext_csd[EXT_CSD_SEC_CNT + 1] << 8 |
- ext_csd[EXT_CSD_SEC_CNT + 2] << 16 |
- ext_csd[EXT_CSD_SEC_CNT + 3] << 24;
- if (card->ext_csd.sectors)
- mmc_card_set_blockaddr(card);
- }
+ card->ext_csd.sectors =
+ ext_csd[EXT_CSD_SEC_CNT + 0] << 0 |
+ ext_csd[EXT_CSD_SEC_CNT + 1] << 8 |
+ ext_csd[EXT_CSD_SEC_CNT + 2] << 16 |
+ ext_csd[EXT_CSD_SEC_CNT + 3] << 24;
+ if (card->ext_csd.sectors)
+ mmc_card_set_blockaddr(card);
switch (ext_csd[EXT_CSD_CARD_TYPE]) {
case EXT_CSD_CARD_TYPE_52 | EXT_CSD_CARD_TYPE_26:
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH][MMC] Fix wrong EXT_CSD_REV handling
2007-12-13 7:13 [PATCH][MMC] Fix wrong EXT_CSD_REV handling Kyungmin Park
@ 2007-12-13 7:53 ` Pierre Ossman
2007-12-13 8:08 ` Kyungmin Park
0 siblings, 1 reply; 6+ messages in thread
From: Pierre Ossman @ 2007-12-13 7:53 UTC (permalink / raw)
To: Kyungmin Park; +Cc: linux-kernel
On Thu, 13 Dec 2007 16:13:11 +0900
Kyungmin Park <kyungmin.park@samsung.com> wrote:
> It already checked the ext_csd_struct is less than 2,
> so it doesn't need to check it.
> Current code only accepts the revision 1.2.
>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
It wasn't wrong the last time you brought this up, and it still isn't wrong. Those bits aren't defined until version 1.2 of the EXT_CSD register, hence we do not trust them. If you have some broken card that you feel you must have support for, then start working on some quirks system.
Rgds
--
-- Pierre Ossman
Linux kernel, MMC maintainer http://www.kernel.org
PulseAudio, core developer http://pulseaudio.org
rdesktop, core developer http://www.rdesktop.org
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][MMC] Fix wrong EXT_CSD_REV handling
2007-12-13 7:53 ` Pierre Ossman
@ 2007-12-13 8:08 ` Kyungmin Park
2007-12-13 8:24 ` Pierre Ossman
0 siblings, 1 reply; 6+ messages in thread
From: Kyungmin Park @ 2007-12-13 8:08 UTC (permalink / raw)
To: Pierre Ossman; +Cc: linux-kernel
On Dec 13, 2007 4:53 PM, Pierre Ossman <drzeus-list@drzeus.cx> wrote:
> On Thu, 13 Dec 2007 16:13:11 +0900
> Kyungmin Park <kyungmin.park@samsung.com> wrote:
>
> > It already checked the ext_csd_struct is less than 2,
> > so it doesn't need to check it.
> > Current code only accepts the revision 1.2.
> >
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>
> It wasn't wrong the last time you brought this up, and it still isn't wrong. Those bits aren't defined until version 1.2 of the EXT_CSD register, hence we do not trust them. If you have some broken card that you feel you must have support for, then start working on some quirks system.
Now see the code.
First, it checks the ext_csd_struct.
ext_csd_struct = ext_csd[EXT_CSD_REV];
if (ext_csd_struct > 2) {
printk(KERN_ERR "%s: unrecognised EXT_CSD structure "
"version %d\n", mmc_hostname(card->host),
ext_csd_struct);
err = -EINVAL;
goto out;
}
=> here possible values are 0, 1, 2
=> It only accepts the ext_csd 2 but my *broken* card has 1.
so it's failed.
if (ext_csd_struct >= 2) {
card->ext_csd.sectors =
ext_csd[EXT_CSD_SEC_CNT + 0] << 0 |
ext_csd[EXT_CSD_SEC_CNT + 1] << 8 |
ext_csd[EXT_CSD_SEC_CNT + 2] << 16 |
ext_csd[EXT_CSD_SEC_CNT + 3] << 24;
if (card->ext_csd.sectors)
mmc_card_set_blockaddr(card);
}
In my MMC Spec. (v4.2), there's no problem to read it even though it's
revision 1.1
Anyway how do it handle this one? Do you have any idea?
Thank you,
Kyungmin Park
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][MMC] Fix wrong EXT_CSD_REV handling
2007-12-13 8:08 ` Kyungmin Park
@ 2007-12-13 8:24 ` Pierre Ossman
2007-12-14 2:01 ` Kyungmin Park
0 siblings, 1 reply; 6+ messages in thread
From: Pierre Ossman @ 2007-12-13 8:24 UTC (permalink / raw)
To: Kyungmin Park; +Cc: linux-kernel
On Thu, 13 Dec 2007 17:08:16 +0900
"Kyungmin Park" <kmpark@infradead.org> wrote:
>
> In my MMC Spec. (v4.2), there's no problem to read it even though it's
> revision 1.1
>
Well, the spec says that those reserved fields "should be zero". Unfortunately, people seem to have read this in the IETF sense and not as "the fields MUST be zero". I.e. I've seen cards where reserved fields contain junk.
> Anyway how do it handle this one? Do you have any idea?
>
The EXT_CSD is read after the CID, so add some table that maps certain workarounds to specific cards. Then add some bit saying "is really EXT_CSD 1.2" and check for that bit when parsing the EXT_CSD.
Rgds
--
-- Pierre Ossman
Linux kernel, MMC maintainer http://www.kernel.org
PulseAudio, core developer http://pulseaudio.org
rdesktop, core developer http://www.rdesktop.org
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][MMC] Fix wrong EXT_CSD_REV handling
2007-12-13 8:24 ` Pierre Ossman
@ 2007-12-14 2:01 ` Kyungmin Park
2007-12-18 8:04 ` Pierre Ossman
0 siblings, 1 reply; 6+ messages in thread
From: Kyungmin Park @ 2007-12-14 2:01 UTC (permalink / raw)
To: Pierre Ossman; +Cc: linux-kernel
On Dec 13, 2007 5:24 PM, Pierre Ossman <drzeus-list@drzeus.cx> wrote:
> On Thu, 13 Dec 2007 17:08:16 +0900
> "Kyungmin Park" <kmpark@infradead.org> wrote:
>
> >
> > In my MMC Spec. (v4.2), there's no problem to read it even though it's
> > revision 1.1
> >
>
> Well, the spec says that those reserved fields "should be zero". Unfortunately, people seem to have read this in the IETF sense and not as "the fields MUST be zero". I.e. I've seen cards where reserved fields contain junk.
>
Yes, reserved word should or must be zero, then it should check "if
(ext_csd_struct <= 2)" instead of ">= 2".
In the Spec. 4.2, it can have three value 0, 1, or 2. There's no other
restriction.
Thank you,
Kyungmin Park.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][MMC] Fix wrong EXT_CSD_REV handling
2007-12-14 2:01 ` Kyungmin Park
@ 2007-12-18 8:04 ` Pierre Ossman
0 siblings, 0 replies; 6+ messages in thread
From: Pierre Ossman @ 2007-12-18 8:04 UTC (permalink / raw)
To: Kyungmin Park; +Cc: linux-kernel
On Fri, 14 Dec 2007 11:01:03 +0900
"Kyungmin Park" <kmpark@infradead.org> wrote:
>
> Yes, reserved word should or must be zero, then it should check "if
> (ext_csd_struct <= 2)" instead of ">= 2".
> In the Spec. 4.2, it can have three value 0, 1, or 2. There's no other
> restriction.
As I said, the spec doesn't say "must", and I've seen cards with bogus data in there so I'm afraid we can't rely on the fields being zero. The version field is the only decent way of determining what to expect in the rest of the structure.
Rgds
--
-- Pierre Ossman
Linux kernel, MMC maintainer http://www.kernel.org
PulseAudio, core developer http://pulseaudio.org
rdesktop, core developer http://www.rdesktop.org
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2007-12-18 8:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-12-13 7:13 [PATCH][MMC] Fix wrong EXT_CSD_REV handling Kyungmin Park
2007-12-13 7:53 ` Pierre Ossman
2007-12-13 8:08 ` Kyungmin Park
2007-12-13 8:24 ` Pierre Ossman
2007-12-14 2:01 ` Kyungmin Park
2007-12-18 8:04 ` Pierre Ossman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.