On Tuesday 01 March 2011, Andrei Warkentin wrote:
> Quirks are card-specific workarounds. Usually they involve
> tuning mmcblk parameters at mmc_blk_probe time, but can
> involve affecting the way mmcblk I/O requests are handled.
> The later is necessary to handle Sandisk out-of-spec discard support,
> and the small-writes reliability workaround for Toshiba.
I was hoping for a simpler method, let me elaborate below
> Change-Id: Ia5e56d7a2803daba4b9085f2ca12afeadc7d9095
We don't put a Change-Id into kernel patches, they are meaningless.
> +struct gendisk;
> +
> +#ifdef CONFIG_MMC_BLOCK_QUIRKS
> +struct mmc_blk_data;
> +
> +#define mmc_blk_qrev(hwrev, fwrev, year, month) \
> + (((u64) hwrev) << 40 | \
> + ((u64) fwrev) << 32 | \
> + ((u64) year) << 16 | \
> + ((u64) month))
> +
> +#define mmc_blk_qrev_card(card) \
> + mmc_blk_qrev(card->cid.hwrev, \
> + card->cid.fwrev, \
> + card->cid.year, \
> + card->cid.month)
> +
> +struct mmc_blk_quirk {
> + struct rb_node rb_node;
> + const char *name;
> +
> + /* First valid revision */
> + u64 rev_start;
> +
> + /* Last valid revision */
> + u64 rev_end;
> +
> + unsigned int manfid;
> + unsigned short oemid;
> + int (*probe)(struct mmc_blk_data *, struct mmc_card *);
So far so good.
> + int (*adjust)(struct mmc_queue *, struct request *, struct mmc_request
> *);
> +};
The adjust function looks out of place here, I would leave it out until it
really is used.
> +/*
> + * There is one mmc_blk_data per slot.
> + */
> +struct mmc_blk_data {
> + spinlock_t lock;
> + struct gendisk *disk;
> + struct mmc_queue queue;
> +
> + unsigned int usage;
> + unsigned int read_only;
> + unsigned int write_align_size;
> +#ifdef CONFIG_MMC_BLOCK_QUIRKS
> + struct mmc_blk_quirk *quirk;
> +#endif /* CONFIG_MMC_BLOCK_QUIRKS */
> +};
I don't think a pointer to the quirk is needed here, you can probably
leave the mmc_blk_data alone.
> +/*
> + Since the goal is to support quirks in removable media, ideally
> + such quirks would be always built into a kernel, hence we need
> + a smarter way to search.
> +*/
> +static struct rb_root quirk_tree[2] = { RB_ROOT, RB_ROOT };
> +
> +static int mmc_blk_quirk_cmp(const struct mmc_blk_quirk *quirk,
> + const struct mmc_blk_quirk *cquirk)
> +{
> + int ret;
> +
> + if (quirk->manfid > cquirk->manfid)
> + return 1;
> + else if (quirk->manfid < cquirk->manfid)
> + return -1;
> +
> + if (quirk->oemid > cquirk->oemid)
> + return 1;
> + else if (quirk->oemid < cquirk->oemid)
> + return -1;
> +
> + ret = strcmp(quirk->name, cquirk->name);
> + if (!ret)
> + return ret;
> +
> + if (quirk->rev_start > cquirk->rev_end)
> + return 1;
> + else if (quirk->rev_end < cquirk->rev_start)
> + return -1;
> +
> + /* Overlap in revs or equal. */
> + return 0;
> +}
> +
> +static int mmc_blk_quirk_cmpc(const struct mmc_blk_quirk *quirk,
> + const struct mmc_card *card)
> +{
> + struct mmc_blk_quirk cquirk;
> + cquirk.name = card->cid.prod_name;
> + cquirk.rev_start = mmc_blk_qrev_card(card);
> + cquirk.rev_end = cquirk.rev_start;
> + cquirk.manfid = card->cid.manfid;
> + cquirk.oemid = card->cid.oemid;
> + return mmc_blk_quirk_cmp(quirk, &cquirk);
> +}
This can be simplified slightly if you avoid calling mmc_blk_quirk_cmp.
> +struct mmc_blk_quirk *mmc_blk_quirk_find(struct mmc_card *card)
> +{
> + int ret;
> + struct mmc_blk_quirk *quirk;
> + struct rb_node *node = quirk_tree[!mmc_card_mmc(card)].rb_node;
> +
> + while (node) {
> + quirk = container_of(node, struct mmc_blk_quirk, rb_node);
> + ret = mmc_blk_quirk_cmpc(quirk, card);
> +
> + if (ret < 0)
> + node = node->rb_left;
> + else if (ret > 0)
> + node = node->rb_right;
> + else
> + return quirk;
> + }
> +
> + return NULL;
> +}
> +
> +int mmc_blk_quirk_register(struct mmc_blk_quirk *quirk, bool is_mmc)
> +{
> + int ret;
> + struct mmc_blk_quirk *cq;
> + struct rb_node **new = &(quirk_tree[!is_mmc].rb_node), *parent = NULL;
> +
> + while (*new) {
> + cq = container_of(*new, struct mmc_blk_quirk, rb_node);
> +
> + ret = mmc_blk_quirk_cmp(quirk, cq);
> +
> + parent = *new;
> + if (ret < 0)
> + new = &(parent->rb_left);
> + else if(ret > 0)
> + new = &(parent->rb_right);
> + else
> + /* Overlap */
> + return -EINVAL;
> +
> + }
> +
> + rb_link_node(&quirk->rb_node, parent, new);
> + rb_insert_color(&quirk->rb_node, &quirk_tree[!is_mmc]);
> + return 0;
> +}
Instead of the dynamic registration, I'd just put all quirks into
one file and have an array of them:
#define MMC_BLK_QUIRK_VERSION(_name, _manfid, _oemid, _rev_start, _rev_end,
_probe) \
{ \
.name = (_name), \
.manfid = (_manfid), \
.oemid = (_oemid), \
.rev_start = (_rev_start),\
.rev_end = (_rev_end), \
.probe = (_probe), \
}
#define MMC_BLK_QUIRK(_name, _manfid, _oemid, _probe) \
MMC_BLK_QUIRK_VERSION(_name, _manfid, _oemid, 0, -1ull, _probe)
struct mmc_blk_quirk mmc_blk_quirks[] = {
MMC_BLK_QUIRK("MMC32G", 0x11, 0x0100, toshiba_mmc32g),
...
};
> +#ifdef CONFIG_MMC_BLOCK_QUIRKS
> + if (md->quirk && md->quirk->adjust)
> + md->quirk->adjust(mq, req, &brq.mrq);
> +#endif /* CONFIG_MMC_BLOCK_QUIRKS */
Instead of having a function pointer here, I'd start out with
an open-coded version of the one function that you know is required,
and make it depend on members of struct mmc_card that can get
set in the .probe function.
> +#ifdef CONFIG_MMC_BLOCK_QUIRKS
> + md->quirk = mmc_blk_quirk_find(card);
> + if (md->quirk && md->quirk->probe)
> + err = quirk->probe(md, card);
> + if (err)
> + goto out;
> +#endif /* CONFIG_MMC_BLOCK_QUIRKS */
Just make this an inline function in the header and call it unconditionally:
#ifdef CONFIG_MMC_BLOCK_QUIRKS
extern int mmc_blk_quirk_find(struct mmc_card *card);
#else
static inline int mmc_blk_quirk_find(struct mmc_card *card)
{
return 0;
}
#endif
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
