Re: [PATCH] memstick: rtsx: fix ms card data transfer bug

[Andrew Morton]

On Wed, 6 Nov 2013 09:14:59 +0800 micky <micky_ching@xxxxxxxxxxxxxx> wrote:

> On 11/06/2013 05:10 AM, Andrew Morton wrote:
> > On Wed, 30 Oct 2013 14:40:16 +0800 <micky_ching@xxxxxxxxxxxxxx> wrote:
> >
> >> unlike mspro card, ms card use normal read/write mode for DMA
> >> data transfer.
> > What are the user-visible effects of this bug?
> >
> > Please always include this information when fixing bugs so that others
> > can decide whether they (or their customers) need the patch.
> >
>

(top-posting repaired - please don't top-post!)

> MS card can not use auto read/write mode, so it will fail at
> initialize and long data transfer. This patch is used to add
> support for ms card.
> 
> Shall I re-send this patch to add more info?

That's OK - I updated the changelog in-place and added cc:stable so it
gets backported.  But then I dropped the patch ;)

>From this info I assume that use of ms cards is very rare, otherwise
people would have complained.  What is the difference between an "ms
card" and an "mspro card"?  How common are each type and what is their
availability?

ms_transfer_data() and mspro_transfer_data() are very similar.  I think
it would be more maintainable if they were integrated into a single
function?

trans_done and timeleft could be made local to the code block where
they used.  This would be neater and more maintainable.

This code is troublesome:

: 	if (pcr->trans_result == TRANS_NOT_READY) {
: 		init_completion(&trans_done);
: 		timeleft = wait_for_completion_interruptible_timeout(
: 			&trans_done, 1000);
: 		if (timeleft < 0) {
: 			dev_dbg(ms_dev(host),
: 				"%s: timeout wait for ok interrupt.\n",
: 				__func__);
: 			return -ETIMEDOUT;
: 		}
: 	}

- Why does it exist?  Needs a comment explaining what it is trying to
  achieve.

- It should use DECLARE_COMPLETION_ONSTACK() for trans_done

- It uses wait_for_completion() but nothing ever calls complete() on
  the object!  That's just bizarre and more appropriate primitives
  should be used.

- The debug message is hard to understand and appears to be wrong. 
  Should be "interrupt received while waiting for <something?>".

- The code appears to be terminating a kernel IO transaction when the
  user hits ^C.  That's just not viable - the ^C could have been
  entered for other reasons and the IO will complete just fine.  Also
  no -EINTR is returned callers don't appear to be set up to handle it.

So shudder.  I'll drop the patch.  Please explain very carefully what
you're trying to achieve here and perhaps we can suggest a suitable
implementation approach.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/