Re: [PATCH v3] mmc: Add ioctl to let userspace apps send ACMDs

[John Calixto <john.calixto@modsystems.com>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2749 bytes --]

Hi Andrei,

On Fri, 8 Apr 2011, Andrei Warkentin wrote:

> Hi John,
> 
> On Thu, Apr 7, 2011 at 8:18 PM, John Calixto
> <john.calixto@modsystems.com> wrote:
> > +       /* data.flags must already be set before doing this. */
> > +       mmc_set_data_timeout(&data, card);
> > +       /* Allow overriding the timeout_ns for empirical tuning. */
> > +       if (idata->ic.force_timeout_ns)
> > +               data.timeout_ns = idata->ic.force_timeout_ns;
> > +
> 
> Are there any R1B ACMDs?
> 

Yes, there are.  Do you think this timeout is unnecessary?  We found it
useful to adjust this to be more accepting of some cheaper cards that
exceeded the timeout in the spec (250 ms).

> > +       mmc_wait_for_req(card->host, &mrq);
> > +
> > +       if (cmd.error) {
> > +               dev_err(mmc_dev(card->host), "%s: cmd error %d\n",
> > +                                               __func__, cmd.error);
> > +               err = cmd.error;
> > +               goto acmd_rel_host;
> > +       }
> > +       if (data.error) {
> > +               dev_err(mmc_dev(card->host), "%s: data error %d\n",
> > +                                               __func__, data.error);
> > +               err = data.error;
> > +               goto acmd_rel_host;
> > +       }
> > +
> > +       /*
> > +        * According to the SD specs, some commands require a delay after
> > +        * issuing the command.
> > +        */
> > +       if (idata->ic.postsleep_us)
> > +               udelay(idata->ic.postsleep_us);
> > +
> 
> How long can this possibly be? Does usleep make any sense here?
> 

In practice, 100 us.  Borderline?  Note that this sleep is separate from
the timeout above.  This one is specifically for read operations where
there is no such thing as "busy".

> >
>  +struct mmc_ioc_cmd {
> > +       int write_flag;  /* implies direction of data.  true = write, false = read */
> > +       __u32 opcode;
> > +       __u32 arg;
> > +       __u32 response[4];  /* CMD response */
> > +       unsigned int flags;
> > +       unsigned int blksz;
> > +       unsigned int blocks;
> 
> I'm not sure I understand why blksz is passed. If you needed to set
> the block size (via CMD16) prior to some command (like MMC password
> protection) I could kind of get it, but then all I see the blk size
> getting used for is calculating the data amount in bytes, so you're
> not doing that. So why not just infer it from the card queue?
> 

The block size for some commands is strictly specified, and does
change according to the command.  In these cases, you do not call CMD16
to change it.  You just call the command knowing that its block size is
fixed according to spec.

John