Re: [PATCH] memstick: add support for legacy memorysticks
On Mon, 8 Jul 2013 23:54:55 +0300 Maxim Levitsky
wrote:
> Based partially on MS standard spec quotes from Alex Dubov.
>
> As any code that works with user data this driver isn't
> recommended to use to write cards that contain valuable data.
>
> It tries its best though to avoid data corruption
> and possible damage to the card.
>
> Tested on MS DUO 64 MB card on Ricoh R592 card reader.
>
> ...
>
> +config MS_BLOCK
> + tristate "MemoryStick Standard device driver"
> + depends on BLOCK
> + help
> + Say Y here to enable the MemoryStick Standard device driver
> + support. This provides a block device driver, which you can use
> + to mount the filesystem.
> + This driver works with old (bulky) MemoryStick and MemoryStick Duo
> + but not PRO. Say Y if you have such card.
> + Driver is new and not yet well tested, thus it can damage your card
> + (even permanently)
Yikes. How true is this?
>
> ...
>
> +static size_t msb_sg_copy(struct scatterlist *sg_from,
> + struct scatterlist *sg_to, int to_nents, size_t offset, size_t len)
> +{
> + size_t copied = 0;
> +
> + while (offset > 0) {
> + if (offset >= sg_from->length) {
> + if (sg_is_last(sg_from))
> + return 0;
> +
> + offset -= sg_from->length;
> + sg_from = sg_next(sg_from);
> + continue;
> + }
> +
> + copied = min(len, sg_from->length - offset);
> + sg_set_page(sg_to, sg_page(sg_from),
> + copied, sg_from->offset + offset);
> +
> + len -= copied;
> + offset = 0;
> +
> + if (sg_is_last(sg_from) || !len)
> + goto out;
> +
> + sg_to = sg_next(sg_to);
> + to_nents--;
> + sg_from = sg_next(sg_from);
> + }
> +
> + while (len > sg_from->length && to_nents--) {
> + len -= sg_from->length;
> + copied += sg_from->length;
> +
> + sg_set_page(sg_to, sg_page(sg_from),
> + sg_from->length, sg_from->offset);
> +
> + if (sg_is_last(sg_from) || !len)
> + goto out;
> +
> + sg_from = sg_next(sg_from);
> + sg_to = sg_next(sg_to);
> + }
> +
> + if (len && to_nents) {
> + sg_set_page(sg_to, sg_page(sg_from), len, sg_from->offset);
> + copied += len;
> + }
> +out:
> + sg_mark_end(sg_to);
> + return copied;
> +}
There's nothing memstick-specific about this. Did you consider placing
it in lib/?
> +/*
> + * Compares section of 'sg' starting from offset 'offset' and with length
> 'len'
> + * to linear buffer of length 'len' at address 'buffer'
> + * Returns 0 if equal and -1 otherwice
> + */
> +static int msb_sg_compare_to_buffer(struct scatterlist *sg,
> + size_t offset, u8 *buffer, size_t len)
> +{
> + int retval = 0, cmplen;
> + struct sg_mapping_iter miter;
> +
> + sg_miter_start(, sg, sg_nents(sg),
> + SG_MITER_ATOMIC | SG_MITER_FROM_SG);
> +
> + while (sg_miter_next() && len > 0) {
> + if (offset >= miter.length) {
> + offset -= miter.length;
> + continue;
> + }
> +
> + cmplen = min(miter.length - offset, len);
> + retval = memcmp(miter.addr + offset, buffer, cmplen) ? -1 : 0;
> + if (retval)
> + break;
> +
> + buffer += cmplen;
> + len -= cmplen;
> + offset = 0;
> + }
> +
> + if (!retval && len)
> + retval = -1;
> +
> + sg_miter_stop();
> + return retval;
> +}
And this, perhaps.
>
> ...
>
> +static void msb_mark_block_used(struct msb_data *msb, int pba)
> +{
> + int zone = msb_get_zone_from_pba(pba);
> +
> + if (test_bit(pba, msb->used_blocks_bitmap)) {
> + pr_err(
> + "BUG: attempt to mark already used pba %d as used", pba);
> + msb->read_only = true;
> + return;
> + }
> +
> + if (msb_validate_used_block_bitmap(msb))
> + return;
> +
> + /* No races because all IO is single threaded */
What guarantees that all IO is single threaded? Big lock? All done by
a single kernel thread?
> + __set_bit(pba, msb->used_blocks_bitmap);
> + msb->free_block_count[zone]--;
> +}
> +
>
> ...
>
> +static int msb_read_int_reg(struct msb_data *msb, long timeout)
> +{
> + struct memstick_request *mrq = >card->current_mrq;
> +
> + WARN_ON(msb->state == -1);
> +
> + if (!msb->int_polling) {
> + msb->int_timeout = jiffies +
> + msecs_to_jiffies(timeout == -1 ? 500 : timeout);
All callers pass timeout=-1, so there's some pointless code here.
> + msb->int_polling = true;
>
Re: [PATCH] memstick: add support for legacy memorysticks
On Mon, 8 Jul 2013 23:54:55 +0300 Maxim Levitsky maximlevit...@gmail.com
wrote:
Based partially on MS standard spec quotes from Alex Dubov.
As any code that works with user data this driver isn't
recommended to use to write cards that contain valuable data.
It tries its best though to avoid data corruption
and possible damage to the card.
Tested on MS DUO 64 MB card on Ricoh R592 card reader.
...
+config MS_BLOCK
+ tristate MemoryStick Standard device driver
+ depends on BLOCK
+ help
+ Say Y here to enable the MemoryStick Standard device driver
+ support. This provides a block device driver, which you can use
+ to mount the filesystem.
+ This driver works with old (bulky) MemoryStick and MemoryStick Duo
+ but not PRO. Say Y if you have such card.
+ Driver is new and not yet well tested, thus it can damage your card
+ (even permanently)
Yikes. How true is this?
...
+static size_t msb_sg_copy(struct scatterlist *sg_from,
+ struct scatterlist *sg_to, int to_nents, size_t offset, size_t len)
+{
+ size_t copied = 0;
+
+ while (offset 0) {
+ if (offset = sg_from-length) {
+ if (sg_is_last(sg_from))
+ return 0;
+
+ offset -= sg_from-length;
+ sg_from = sg_next(sg_from);
+ continue;
+ }
+
+ copied = min(len, sg_from-length - offset);
+ sg_set_page(sg_to, sg_page(sg_from),
+ copied, sg_from-offset + offset);
+
+ len -= copied;
+ offset = 0;
+
+ if (sg_is_last(sg_from) || !len)
+ goto out;
+
+ sg_to = sg_next(sg_to);
+ to_nents--;
+ sg_from = sg_next(sg_from);
+ }
+
+ while (len sg_from-length to_nents--) {
+ len -= sg_from-length;
+ copied += sg_from-length;
+
+ sg_set_page(sg_to, sg_page(sg_from),
+ sg_from-length, sg_from-offset);
+
+ if (sg_is_last(sg_from) || !len)
+ goto out;
+
+ sg_from = sg_next(sg_from);
+ sg_to = sg_next(sg_to);
+ }
+
+ if (len to_nents) {
+ sg_set_page(sg_to, sg_page(sg_from), len, sg_from-offset);
+ copied += len;
+ }
+out:
+ sg_mark_end(sg_to);
+ return copied;
+}
There's nothing memstick-specific about this. Did you consider placing
it in lib/?
+/*
+ * Compares section of 'sg' starting from offset 'offset' and with length
'len'
+ * to linear buffer of length 'len' at address 'buffer'
+ * Returns 0 if equal and -1 otherwice
+ */
+static int msb_sg_compare_to_buffer(struct scatterlist *sg,
+ size_t offset, u8 *buffer, size_t len)
+{
+ int retval = 0, cmplen;
+ struct sg_mapping_iter miter;
+
+ sg_miter_start(miter, sg, sg_nents(sg),
+ SG_MITER_ATOMIC | SG_MITER_FROM_SG);
+
+ while (sg_miter_next(miter) len 0) {
+ if (offset = miter.length) {
+ offset -= miter.length;
+ continue;
+ }
+
+ cmplen = min(miter.length - offset, len);
+ retval = memcmp(miter.addr + offset, buffer, cmplen) ? -1 : 0;
+ if (retval)
+ break;
+
+ buffer += cmplen;
+ len -= cmplen;
+ offset = 0;
+ }
+
+ if (!retval len)
+ retval = -1;
+
+ sg_miter_stop(miter);
+ return retval;
+}
And this, perhaps.
...
+static void msb_mark_block_used(struct msb_data *msb, int pba)
+{
+ int zone = msb_get_zone_from_pba(pba);
+
+ if (test_bit(pba, msb-used_blocks_bitmap)) {
+ pr_err(
+ BUG: attempt to mark already used pba %d as used, pba);
+ msb-read_only = true;
+ return;
+ }
+
+ if (msb_validate_used_block_bitmap(msb))
+ return;
+
+ /* No races because all IO is single threaded */
What guarantees that all IO is single threaded? Big lock? All done by
a single kernel thread?
+ __set_bit(pba, msb-used_blocks_bitmap);
+ msb-free_block_count[zone]--;
+}
+
...
+static int msb_read_int_reg(struct msb_data *msb, long timeout)
+{
+ struct memstick_request *mrq = msb-card-current_mrq;
+
+ WARN_ON(msb-state == -1);
+
+ if (!msb-int_polling) {
+ msb-int_timeout = jiffies +
+ msecs_to_jiffies(timeout == -1 ? 500 : timeout);
All callers pass timeout=-1, so there's some pointless code here.
+ msb-int_polling = true;
+ } else if (time_after(jiffies, msb-int_timeout)) {
+ mrq-data[0] = MEMSTICK_INT_CMDNAK;
+ return 0;
+ }
+
+
Re: [PATCH] memstick: add support for legacy memorysticks
On Thu, 11 Jul 2013 03:28:37 +0300 Maxim Levitsky wrote: > On Mon, 2013-07-08 at 23:54 +0300, Maxim Levitsky wrote: > > Based partially on MS standard spec quotes from Alex Dubov. > > > > As any code that works with user data this driver isn't > > recommended to use to write cards that contain valuable data. > > > > It tries its best though to avoid data corruption > > and possible damage to the card. > > > > Tested on MS DUO 64 MB card on Ricoh R592 card reader. > > (top-posting repaired. Please don't do that). > Any update? You sent a large patch in the middle of the 3.11 merge window, when nobody is looking at large new patches. I have it saved away for consideration after -rc1. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] memstick: add support for legacy memorysticks
On Thu, 11 Jul 2013 03:28:37 +0300 Maxim Levitsky maximlevit...@gmail.com wrote: On Mon, 2013-07-08 at 23:54 +0300, Maxim Levitsky wrote: Based partially on MS standard spec quotes from Alex Dubov. As any code that works with user data this driver isn't recommended to use to write cards that contain valuable data. It tries its best though to avoid data corruption and possible damage to the card. Tested on MS DUO 64 MB card on Ricoh R592 card reader. (top-posting repaired. Please don't do that). Any update? You sent a large patch in the middle of the 3.11 merge window, when nobody is looking at large new patches. I have it saved away for consideration after -rc1. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] memstick: add support for legacy memorysticks
Hello, On Mon, Sep 24, 2012 at 08:24:37PM +0200, Maxim Levitsky wrote: > Should have looked through the source. Understand now. > Just one quick question, should I create my own workqueue or use > schedule_work? if I use the later and my work function sleeps, will it > harmfully affect other users of this function? If memstick can be in memory reclaim path (most block devices are), you would want to create a dedicated workqueue w/ rescuer. For details, please read Documentation/workqueue.txt. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] memstick: add support for legacy memorysticks
Hello, On Mon, Sep 24, 2012 at 08:19:28PM +0200, Maxim Levitsky wrote: > Except that if I schedule a same work item few times, these work items > will be 'processed' in parallel, although there is just one work to do, > work of pulling the requests from block queue until it has them, and > dispatching them through my code. > Or I can get a guarantee that work items wont be processed in parallel? You need to use system_nrt_wq for that before 3.7-rc1. After 3.7-rc1, any workqueue will guarantee that. > Stiil, even with that only first work item will do the actual work, > others will wake the workqueue for nothing, but I am ok with that. It's just like waking up spuriously. The work item is guaranteed to be executed at least once after any given schedule/queue_work() call. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] memstick: add support for legacy memorysticks
On Mon, 2012-09-24 at 20:19 +0200, Maxim Levitsky wrote: > On Mon, 2012-09-24 at 11:05 -0700, Tejun Heo wrote: > > Hello, > > > > On Mon, Sep 24, 2012 at 05:09:23PM +0200, Maxim Levitsky wrote: > > > > Now that my exams done > > > > Can you spare me from using a workqueue? > > > > I'd much prefer if you convert to workqueue. > > > > > > The point is that using current model I wake the worker thread as much > > > > as I want to, and I know that it will be woken once an will do all the > > > > work till request queue is empty. > > > > You can do exactly the same thing by scheduling the same work item > > multiple times. "Waking up" just becomes "scheduling the work item". > I don't believe that will work this way. > I will dig through the source, and see how to do that. > > > > > > > With workqueues, it doesn't work this way. I have to pass the request as > > > > a work item or something like that. > > > > Any pointers? > > > > No, there's no reason to change the structure of the code in any way. > > Just use a work item as you would use a kthread. > Except that if I schedule a same work item few times, these work items > will be 'processed' in parallel, although there is just one work to do, > work of pulling the requests from block queue until it has them, and > dispatching them through my code. > Or I can get a guarantee that work items wont be processed in parallel? > Stiil, even with that only first work item will do the actual work, > others will wake the workqueue for nothing, but I am ok with that. Should have looked through the source. Understand now. Just one quick question, should I create my own workqueue or use schedule_work? if I use the later and my work function sleeps, will it harmfully affect other users of this function? -- Best regards, Maxim Levitsky -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] memstick: add support for legacy memorysticks
On Mon, 2012-09-24 at 11:05 -0700, Tejun Heo wrote: > Hello, > > On Mon, Sep 24, 2012 at 05:09:23PM +0200, Maxim Levitsky wrote: > > > Now that my exams done > > > Can you spare me from using a workqueue? > > I'd much prefer if you convert to workqueue. > > > > The point is that using current model I wake the worker thread as much > > > as I want to, and I know that it will be woken once an will do all the > > > work till request queue is empty. > > You can do exactly the same thing by scheduling the same work item > multiple times. "Waking up" just becomes "scheduling the work item". I don't believe that will work this way. I will dig through the source, and see how to do that. > > > With workqueues, it doesn't work this way. I have to pass the request as > > > a work item or something like that. > > > Any pointers? > > No, there's no reason to change the structure of the code in any way. > Just use a work item as you would use a kthread. Except that if I schedule a same work item few times, these work items will be 'processed' in parallel, although there is just one work to do, work of pulling the requests from block queue until it has them, and dispatching them through my code. Or I can get a guarantee that work items wont be processed in parallel? Stiil, even with that only first work item will do the actual work, others will wake the workqueue for nothing, but I am ok with that. > > > Also probably due to that reason MMC doesn't use a workqueue ether, but > > a raw kthread, in pretty much same way I do. > > Mostly because I haven't gotten around to convert it yet. The > problems with direct kthread usage are that they're much more > difficult to get completely correct with freeze and exit conditions - Agreed, I had my own share of frustration with this. I got it right in the end though. > the last time I checked it was easier to spot broken ones than correct > ones - and they create dedicated threads which usually are > underutilized. True too, although, a sleeping thread doesn't cost much. > > Thanks. > -- Best regards, Maxim Levitsky -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] memstick: add support for legacy memorysticks
Hello, On Mon, Sep 24, 2012 at 05:09:23PM +0200, Maxim Levitsky wrote: > > Now that my exams done > > Can you spare me from using a workqueue? I'd much prefer if you convert to workqueue. > > The point is that using current model I wake the worker thread as much > > as I want to, and I know that it will be woken once an will do all the > > work till request queue is empty. You can do exactly the same thing by scheduling the same work item multiple times. "Waking up" just becomes "scheduling the work item". > > With workqueues, it doesn't work this way. I have to pass the request as > > a work item or something like that. > > Any pointers? No, there's no reason to change the structure of the code in any way. Just use a work item as you would use a kthread. > Also probably due to that reason MMC doesn't use a workqueue ether, but > a raw kthread, in pretty much same way I do. Mostly because I haven't gotten around to convert it yet. The problems with direct kthread usage are that they're much more difficult to get completely correct with freeze and exit conditions - the last time I checked it was easier to spot broken ones than correct ones - and they create dedicated threads which usually are underutilized. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] memstick: add support for legacy memorysticks
On Mon, 2012-09-24 at 16:59 +0200, Maxim Levitsky wrote: > On Thu, 2012-09-20 at 10:53 -0700, Tejun Heo wrote: > > On Thu, Sep 20, 2012 at 07:05:42AM +0300, Maxim Levitsky wrote: > > > There can't be races in the driver, since it contains a single thread > > > that does all the IO it got from block layer. > > > The thread is awaken each time the request function of block device is > > > called. > > > This why I didn't do much locking in here. In addition I found out that > > > this is quite common way to implement a block device driver. > > > > Please use workqueue instead of raw kthread. > > > > Thanks! > Now that my exams done > Can you spare me from using a workqueue? > The point is that using current model I wake the worker thread as much > as I want to, and I know that it will be woken once an will do all the > work till request queue is empty. > With workqueues, it doesn't work this way. I have to pass the request as > a work item or something like that. > Any pointers? Also probably due to that reason MMC doesn't use a workqueue ether, but a raw kthread, in pretty much same way I do. -- Best regards, Maxim Levitsky -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] memstick: add support for legacy memorysticks
On Thu, 2012-09-20 at 10:53 -0700, Tejun Heo wrote: > On Thu, Sep 20, 2012 at 07:05:42AM +0300, Maxim Levitsky wrote: > > There can't be races in the driver, since it contains a single thread > > that does all the IO it got from block layer. > > The thread is awaken each time the request function of block device is > > called. > > This why I didn't do much locking in here. In addition I found out that > > this is quite common way to implement a block device driver. > > Please use workqueue instead of raw kthread. > > Thanks! Now that my exams done Can you spare me from using a workqueue? The point is that using current model I wake the worker thread as much as I want to, and I know that it will be woken once an will do all the work till request queue is empty. With workqueues, it doesn't work this way. I have to pass the request as a work item or something like that. Any pointers? Best regards, Maxim Levitsky -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] memstick: add support for legacy memorysticks
On Thu, 2012-09-20 at 10:53 -0700, Tejun Heo wrote: On Thu, Sep 20, 2012 at 07:05:42AM +0300, Maxim Levitsky wrote: There can't be races in the driver, since it contains a single thread that does all the IO it got from block layer. The thread is awaken each time the request function of block device is called. This why I didn't do much locking in here. In addition I found out that this is quite common way to implement a block device driver. Please use workqueue instead of raw kthread. Thanks! Now that my exams done Can you spare me from using a workqueue? The point is that using current model I wake the worker thread as much as I want to, and I know that it will be woken once an will do all the work till request queue is empty. With workqueues, it doesn't work this way. I have to pass the request as a work item or something like that. Any pointers? Best regards, Maxim Levitsky -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] memstick: add support for legacy memorysticks
On Mon, 2012-09-24 at 16:59 +0200, Maxim Levitsky wrote: On Thu, 2012-09-20 at 10:53 -0700, Tejun Heo wrote: On Thu, Sep 20, 2012 at 07:05:42AM +0300, Maxim Levitsky wrote: There can't be races in the driver, since it contains a single thread that does all the IO it got from block layer. The thread is awaken each time the request function of block device is called. This why I didn't do much locking in here. In addition I found out that this is quite common way to implement a block device driver. Please use workqueue instead of raw kthread. Thanks! Now that my exams done Can you spare me from using a workqueue? The point is that using current model I wake the worker thread as much as I want to, and I know that it will be woken once an will do all the work till request queue is empty. With workqueues, it doesn't work this way. I have to pass the request as a work item or something like that. Any pointers? Also probably due to that reason MMC doesn't use a workqueue ether, but a raw kthread, in pretty much same way I do. -- Best regards, Maxim Levitsky -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] memstick: add support for legacy memorysticks
Hello, On Mon, Sep 24, 2012 at 05:09:23PM +0200, Maxim Levitsky wrote: Now that my exams done Can you spare me from using a workqueue? I'd much prefer if you convert to workqueue. The point is that using current model I wake the worker thread as much as I want to, and I know that it will be woken once an will do all the work till request queue is empty. You can do exactly the same thing by scheduling the same work item multiple times. Waking up just becomes scheduling the work item. With workqueues, it doesn't work this way. I have to pass the request as a work item or something like that. Any pointers? No, there's no reason to change the structure of the code in any way. Just use a work item as you would use a kthread. Also probably due to that reason MMC doesn't use a workqueue ether, but a raw kthread, in pretty much same way I do. Mostly because I haven't gotten around to convert it yet. The problems with direct kthread usage are that they're much more difficult to get completely correct with freeze and exit conditions - the last time I checked it was easier to spot broken ones than correct ones - and they create dedicated threads which usually are underutilized. Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] memstick: add support for legacy memorysticks
On Mon, 2012-09-24 at 11:05 -0700, Tejun Heo wrote: Hello, On Mon, Sep 24, 2012 at 05:09:23PM +0200, Maxim Levitsky wrote: Now that my exams done Can you spare me from using a workqueue? I'd much prefer if you convert to workqueue. The point is that using current model I wake the worker thread as much as I want to, and I know that it will be woken once an will do all the work till request queue is empty. You can do exactly the same thing by scheduling the same work item multiple times. Waking up just becomes scheduling the work item. I don't believe that will work this way. I will dig through the source, and see how to do that. With workqueues, it doesn't work this way. I have to pass the request as a work item or something like that. Any pointers? No, there's no reason to change the structure of the code in any way. Just use a work item as you would use a kthread. Except that if I schedule a same work item few times, these work items will be 'processed' in parallel, although there is just one work to do, work of pulling the requests from block queue until it has them, and dispatching them through my code. Or I can get a guarantee that work items wont be processed in parallel? Stiil, even with that only first work item will do the actual work, others will wake the workqueue for nothing, but I am ok with that. Also probably due to that reason MMC doesn't use a workqueue ether, but a raw kthread, in pretty much same way I do. Mostly because I haven't gotten around to convert it yet. The problems with direct kthread usage are that they're much more difficult to get completely correct with freeze and exit conditions - Agreed, I had my own share of frustration with this. I got it right in the end though. the last time I checked it was easier to spot broken ones than correct ones - and they create dedicated threads which usually are underutilized. True too, although, a sleeping thread doesn't cost much. Thanks. -- Best regards, Maxim Levitsky -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] memstick: add support for legacy memorysticks
On Mon, 2012-09-24 at 20:19 +0200, Maxim Levitsky wrote: On Mon, 2012-09-24 at 11:05 -0700, Tejun Heo wrote: Hello, On Mon, Sep 24, 2012 at 05:09:23PM +0200, Maxim Levitsky wrote: Now that my exams done Can you spare me from using a workqueue? I'd much prefer if you convert to workqueue. The point is that using current model I wake the worker thread as much as I want to, and I know that it will be woken once an will do all the work till request queue is empty. You can do exactly the same thing by scheduling the same work item multiple times. Waking up just becomes scheduling the work item. I don't believe that will work this way. I will dig through the source, and see how to do that. With workqueues, it doesn't work this way. I have to pass the request as a work item or something like that. Any pointers? No, there's no reason to change the structure of the code in any way. Just use a work item as you would use a kthread. Except that if I schedule a same work item few times, these work items will be 'processed' in parallel, although there is just one work to do, work of pulling the requests from block queue until it has them, and dispatching them through my code. Or I can get a guarantee that work items wont be processed in parallel? Stiil, even with that only first work item will do the actual work, others will wake the workqueue for nothing, but I am ok with that. Should have looked through the source. Understand now. Just one quick question, should I create my own workqueue or use schedule_work? if I use the later and my work function sleeps, will it harmfully affect other users of this function? -- Best regards, Maxim Levitsky -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] memstick: add support for legacy memorysticks
Hello, On Mon, Sep 24, 2012 at 08:19:28PM +0200, Maxim Levitsky wrote: Except that if I schedule a same work item few times, these work items will be 'processed' in parallel, although there is just one work to do, work of pulling the requests from block queue until it has them, and dispatching them through my code. Or I can get a guarantee that work items wont be processed in parallel? You need to use system_nrt_wq for that before 3.7-rc1. After 3.7-rc1, any workqueue will guarantee that. Stiil, even with that only first work item will do the actual work, others will wake the workqueue for nothing, but I am ok with that. It's just like waking up spuriously. The work item is guaranteed to be executed at least once after any given schedule/queue_work() call. Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] memstick: add support for legacy memorysticks
Hello, On Mon, Sep 24, 2012 at 08:24:37PM +0200, Maxim Levitsky wrote: Should have looked through the source. Understand now. Just one quick question, should I create my own workqueue or use schedule_work? if I use the later and my work function sleeps, will it harmfully affect other users of this function? If memstick can be in memory reclaim path (most block devices are), you would want to create a dedicated workqueue w/ rescuer. For details, please read Documentation/workqueue.txt. Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] memstick: add support for legacy memorysticks
On Thu, Sep 20, 2012 at 07:05:42AM +0300, Maxim Levitsky wrote: > There can't be races in the driver, since it contains a single thread > that does all the IO it got from block layer. > The thread is awaken each time the request function of block device is > called. > This why I didn't do much locking in here. In addition I found out that > this is quite common way to implement a block device driver. Please use workqueue instead of raw kthread. Thanks! -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] memstick: add support for legacy memorysticks
Hello, Andrew.
On Wed, Sep 19, 2012 at 02:52:28PM -0700, Andrew Morton wrote:
> I have a bunch of fairly trivial comments, and one head-scratcher for
> Tejun: what's up with the local-irqs-disabled requirement in sg_miter_next()?
> > +static bool sg_compare_to_buffer(struct scatterlist *sg,
> > + int offset, u8 *buffer, size_t len)
> > +{
> > + unsigned long flags;
> > + int retval = 0;
> > + struct sg_mapping_iter miter;
> > +
> > + local_irq_save(flags);
>
> hm, the "IRQ disabled if SG_MITER_ATOMIC" requirement of
> sg_miter_next() is weird and unexpected.
>
> Tejun's 137d3edb48425f8 added the code whch has this strange
> requirement, but it is unexplained. Wazzup?
Heh, that's long time ago. Trying to remember... so it was written
before kmap_atomic() is converted to stack based implementation and
had to be supplied with the specific KMAP index. Atomic mapping
iterator should be useable from irq context too (and there were other
irq handler paths using KM_BIO_SRC_IRQ as the name implies), so it
couldn't allow irq to happen while mapping is in use. At the time,
having to disable IRQ to use any of KM_*_IRQs was rather self-evident.
I think we now can relax the requirement. All that's required is not
to sleep or preempted while mapped. Something like the following is
in order?
8<
Subject: scatterlist: atomic sg_mapping_iter no longer needs disabling IRQ
SG mapping iterator w/ SG_MITER_ATOMIC set required IRQ disabled
because it originally used KM_BIO_SRC_IRQ to allow use from IRQ
handlers. kmap_atomic() has long been updated to handle stacking
atomic mapping requests on per-cpu basis and only requires not
sleeping while mapped.
Update sg_mapping_iter such that atomic iterators only require
disabling preemption instead of disabling IRQ.
While at it, convert wte weird @ARG@ notations to @ARG in the comment
of sg_miter_start().
Signed-off-by: Tejun Heo
Cc: Andrew Morton
---
lib/scatterlist.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index fadae77..e76d85c 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -404,14 +404,13 @@ EXPORT_SYMBOL(sg_miter_start);
* @miter: sg mapping iter to proceed
*
* Description:
- * Proceeds @miter@ to the next mapping. @miter@ should have been
- * started using sg_miter_start(). On successful return,
- * @miter@->page, @miter@->addr and @miter@->length point to the
- * current mapping.
+ * Proceeds @miter to the next mapping. @miter should have been started
+ * using sg_miter_start(). On successful return, @miter->page,
+ * @miter->addr and @miter->length point to the current mapping.
*
* Context:
- * IRQ disabled if SG_MITER_ATOMIC. IRQ must stay disabled till
- * @miter@ is stopped. May sleep if !SG_MITER_ATOMIC.
+ * Preemption disabled if SG_MITER_ATOMIC. Preemption must stay disabled
+ * till @miter is stopped. May sleep if !SG_MITER_ATOMIC.
*
* Returns:
* true if @miter contains the next mapping. false if end of sg
@@ -465,7 +464,8 @@ EXPORT_SYMBOL(sg_miter_next);
* resources (kmap) need to be released during iteration.
*
* Context:
- * IRQ disabled if the SG_MITER_ATOMIC is set. Don't care otherwise.
+ * Preemption disabled if the SG_MITER_ATOMIC is set. Don't care
+ * otherwise.
*/
void sg_miter_stop(struct sg_mapping_iter *miter)
{
@@ -479,7 +479,7 @@ void sg_miter_stop(struct sg_mapping_iter *miter)
flush_kernel_dcache_page(miter->page);
if (miter->__flags & SG_MITER_ATOMIC) {
- WARN_ON(!irqs_disabled());
+ WARN_ON_ONCE(preemptible());
kunmap_atomic(miter->addr);
} else
kunmap(miter->page);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] memstick: add support for legacy memorysticks
Hello, Andrew.
On Wed, Sep 19, 2012 at 02:52:28PM -0700, Andrew Morton wrote:
I have a bunch of fairly trivial comments, and one head-scratcher for
Tejun: what's up with the local-irqs-disabled requirement in sg_miter_next()?
+static bool sg_compare_to_buffer(struct scatterlist *sg,
+ int offset, u8 *buffer, size_t len)
+{
+ unsigned long flags;
+ int retval = 0;
+ struct sg_mapping_iter miter;
+
+ local_irq_save(flags);
hm, the IRQ disabled if SG_MITER_ATOMIC requirement of
sg_miter_next() is weird and unexpected.
Tejun's 137d3edb48425f8 added the code whch has this strange
requirement, but it is unexplained. Wazzup?
Heh, that's long time ago. Trying to remember... so it was written
before kmap_atomic() is converted to stack based implementation and
had to be supplied with the specific KMAP index. Atomic mapping
iterator should be useable from irq context too (and there were other
irq handler paths using KM_BIO_SRC_IRQ as the name implies), so it
couldn't allow irq to happen while mapping is in use. At the time,
having to disable IRQ to use any of KM_*_IRQs was rather self-evident.
I think we now can relax the requirement. All that's required is not
to sleep or preempted while mapped. Something like the following is
in order?
8
Subject: scatterlist: atomic sg_mapping_iter no longer needs disabling IRQ
SG mapping iterator w/ SG_MITER_ATOMIC set required IRQ disabled
because it originally used KM_BIO_SRC_IRQ to allow use from IRQ
handlers. kmap_atomic() has long been updated to handle stacking
atomic mapping requests on per-cpu basis and only requires not
sleeping while mapped.
Update sg_mapping_iter such that atomic iterators only require
disabling preemption instead of disabling IRQ.
While at it, convert wte weird @ARG@ notations to @ARG in the comment
of sg_miter_start().
Signed-off-by: Tejun Heo t...@kernel.org
Cc: Andrew Morton a...@linux-foundation.org
---
lib/scatterlist.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index fadae77..e76d85c 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -404,14 +404,13 @@ EXPORT_SYMBOL(sg_miter_start);
* @miter: sg mapping iter to proceed
*
* Description:
- * Proceeds @miter@ to the next mapping. @miter@ should have been
- * started using sg_miter_start(). On successful return,
- * @miter@-page, @miter@-addr and @miter@-length point to the
- * current mapping.
+ * Proceeds @miter to the next mapping. @miter should have been started
+ * using sg_miter_start(). On successful return, @miter-page,
+ * @miter-addr and @miter-length point to the current mapping.
*
* Context:
- * IRQ disabled if SG_MITER_ATOMIC. IRQ must stay disabled till
- * @miter@ is stopped. May sleep if !SG_MITER_ATOMIC.
+ * Preemption disabled if SG_MITER_ATOMIC. Preemption must stay disabled
+ * till @miter is stopped. May sleep if !SG_MITER_ATOMIC.
*
* Returns:
* true if @miter contains the next mapping. false if end of sg
@@ -465,7 +464,8 @@ EXPORT_SYMBOL(sg_miter_next);
* resources (kmap) need to be released during iteration.
*
* Context:
- * IRQ disabled if the SG_MITER_ATOMIC is set. Don't care otherwise.
+ * Preemption disabled if the SG_MITER_ATOMIC is set. Don't care
+ * otherwise.
*/
void sg_miter_stop(struct sg_mapping_iter *miter)
{
@@ -479,7 +479,7 @@ void sg_miter_stop(struct sg_mapping_iter *miter)
flush_kernel_dcache_page(miter-page);
if (miter-__flags SG_MITER_ATOMIC) {
- WARN_ON(!irqs_disabled());
+ WARN_ON_ONCE(preemptible());
kunmap_atomic(miter-addr);
} else
kunmap(miter-page);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] memstick: add support for legacy memorysticks
On Thu, Sep 20, 2012 at 07:05:42AM +0300, Maxim Levitsky wrote: There can't be races in the driver, since it contains a single thread that does all the IO it got from block layer. The thread is awaken each time the request function of block device is called. This why I didn't do much locking in here. In addition I found out that this is quite common way to implement a block device driver. Please use workqueue instead of raw kthread. Thanks! -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] memstick: add support for legacy memorysticks
On Wed, 2012-09-19 at 14:52 -0700, Andrew Morton wrote:
> On Wed, 19 Sep 2012 16:19:02 +0300
> Maxim Levitsky wrote:
>
> > Based partially on MS standard spec quotes from Alex Dubov.
> >
> > As any code that works with user data this driver isn't
> > recommended to use to write cards that contain valuable data.
> >
> > It tries its best though to avoid data corruption
> > and possible damage to the card.
> >
> > Tested on MS DUO 64 MB card on Ricoh R592 card reader..
> >
>
> Generally looks nice to me, although I wouldn't know a memstick if one
> stuck to my shoe.
>
> I have a bunch of fairly trivial comments, and one head-scratcher for
> Tejun: what's up with the local-irqs-disabled requirement in sg_miter_next()?
Awesome, thanks for the review!
I will address all comments very soon.
>
> > ...
> >
> > +static int sg_nents(struct scatterlist *sg)
> > +{
> > + int nents = 0;
> > + while (sg) {
> > + nents++;
> > + sg = sg_next(sg);
> > + }
> > +
> > + return nents;
> > +}
>
> I think we may as well put this in scatterlist.c immediately. It
> wouldn't surprise me if we already have open-coded copies of this lying
> around the tree.
I agree, but as usual afraid that it wont be accepted here.
>
> > +/*
> > + * Copies section of 'sg_from' starting from offset 'offset' and with
> > length
> > + * 'len' To another scatterlist of to_nents enties
> > + */
> > +static int sg_copy(struct scatterlist *sg_from, struct scatterlist *sg_to,
> > + int to_nents, int offset, size_t len)
>
> The should return a size_t, to match the type of `len'. And the type
> of `offset' is fishy - there's nothing which intrinsically limits these
> things to 2G?
In my case, I will never have large offsets here, but I agree with you
completely.
>
> > +{
> > + int copied = 0;
> > +
> > + while (offset > 0) {
> > +
> > + if (offset >= sg_from->length) {
> > + if (sg_is_last(sg_from))
> > + return 0;
> > +
> > + offset -= sg_from->length;
> > + sg_from = sg_next(sg_from);
> > + continue;
> > + }
> > +
> > + copied = min(len, (size_t)(sg_from->length - offset));
>
> min_t would be more conventional. Or perhaps even min(), depending on
> `offset's new type (but probably not).
Agree.
>
> > + sg_set_page(sg_to, sg_page(sg_from),
> > + copied, sg_from->offset + offset);
> > +
> > + len -= copied;
> > + offset = 0;
> > +
> > + if (sg_is_last(sg_from) || !len)
> > + goto out;
> > +
> > + sg_to = sg_next(sg_to);
> > + to_nents--;
> > + sg_from = sg_next(sg_from);
> > + }
> > +
> > + while (len > sg_from->length && to_nents--) {
> > +
> > + len -= sg_from->length;
> > + copied += sg_from->length;
> > +
> > + sg_set_page(sg_to, sg_page(sg_from),
> > + sg_from->length, sg_from->offset);
> > +
> > + if (sg_is_last(sg_from) || !len)
> > + goto out;
> > +
> > + sg_from = sg_next(sg_from);
> > + sg_to = sg_next(sg_to);
> > + }
> > +
> > + if (len && to_nents) {
> > + sg_set_page(sg_to, sg_page(sg_from), len, sg_from->offset);
> > + copied += len;
> > + }
> > +
> > +out:
> > + sg_mark_end(sg_to);
> > + return copied;
> > +}
> > +
> > +/*
> > + * Compares section of 'sg' starting from offset 'offset' and with length
> > 'len'
> > + * to linear buffer of length 'len' at address 'buffer'
>
> The comment should document the return value.
>
> > + */
> > +static bool sg_compare_to_buffer(struct scatterlist *sg,
> > + int offset, u8 *buffer, size_t len)
> > +{
> > + unsigned long flags;
> > + int retval = 0;
> > + struct sg_mapping_iter miter;
> > +
> > + local_irq_save(flags);
>
> hm, the "IRQ disabled if SG_MITER_ATOMIC" requirement of
> sg_miter_next() is weird and unexpected.
>
> Tejun's 137d3edb48425f8 added the code whch has this strange
> requirement, but it is unexplained. Wazzup?
>
> > + sg_miter_start(, sg, sg_nents(sg),
> > + SG_MITER_ATOMIC | SG_MITER_FROM_SG);
> > +
> > + while (sg_miter_next() && len > 0) {
> > +
> > + int cmplen;
> > +
> > + if (offset >= miter.length) {
> > + offset -= miter.length;
> > + continue;
> > + }
> > +
> > + cmplen = min(miter.length - offset, len);
> > + retval = memcmp(miter.addr + offset, buffer, cmplen);
> > + if (retval)
> > + break;
> > +
> > + buffer += cmplen;
> > + len -= cmplen;
> > + offset = 0;
> > + }
> > +
> > + if (!retval && len)
> > + retval = -1;
> > +
> > + sg_miter_stop();
> > + local_irq_restore(flags);
>
Re: [PATCH] memstick: add support for legacy memorysticks
On Wed, 19 Sep 2012 16:19:02 +0300
Maxim Levitsky wrote:
> Based partially on MS standard spec quotes from Alex Dubov.
>
> As any code that works with user data this driver isn't
> recommended to use to write cards that contain valuable data.
>
> It tries its best though to avoid data corruption
> and possible damage to the card.
>
> Tested on MS DUO 64 MB card on Ricoh R592 card reader..
>
Generally looks nice to me, although I wouldn't know a memstick if one
stuck to my shoe.
I have a bunch of fairly trivial comments, and one head-scratcher for
Tejun: what's up with the local-irqs-disabled requirement in sg_miter_next()?
> ...
>
> +static int sg_nents(struct scatterlist *sg)
> +{
> + int nents = 0;
> + while (sg) {
> + nents++;
> + sg = sg_next(sg);
> + }
> +
> + return nents;
> +}
I think we may as well put this in scatterlist.c immediately. It
wouldn't surprise me if we already have open-coded copies of this lying
around the tree.
> +/*
> + * Copies section of 'sg_from' starting from offset 'offset' and with length
> + * 'len' To another scatterlist of to_nents enties
> + */
> +static int sg_copy(struct scatterlist *sg_from, struct scatterlist *sg_to,
> + int to_nents, int offset, size_t len)
The should return a size_t, to match the type of `len'. And the type
of `offset' is fishy - there's nothing which intrinsically limits these
things to 2G?
> +{
> + int copied = 0;
> +
> + while (offset > 0) {
> +
> + if (offset >= sg_from->length) {
> + if (sg_is_last(sg_from))
> + return 0;
> +
> + offset -= sg_from->length;
> + sg_from = sg_next(sg_from);
> + continue;
> + }
> +
> + copied = min(len, (size_t)(sg_from->length - offset));
min_t would be more conventional. Or perhaps even min(), depending on
`offset's new type (but probably not).
> + sg_set_page(sg_to, sg_page(sg_from),
> + copied, sg_from->offset + offset);
> +
> + len -= copied;
> + offset = 0;
> +
> + if (sg_is_last(sg_from) || !len)
> + goto out;
> +
> + sg_to = sg_next(sg_to);
> + to_nents--;
> + sg_from = sg_next(sg_from);
> + }
> +
> + while (len > sg_from->length && to_nents--) {
> +
> + len -= sg_from->length;
> + copied += sg_from->length;
> +
> + sg_set_page(sg_to, sg_page(sg_from),
> + sg_from->length, sg_from->offset);
> +
> + if (sg_is_last(sg_from) || !len)
> + goto out;
> +
> + sg_from = sg_next(sg_from);
> + sg_to = sg_next(sg_to);
> + }
> +
> + if (len && to_nents) {
> + sg_set_page(sg_to, sg_page(sg_from), len, sg_from->offset);
> + copied += len;
> + }
> +
> +out:
> + sg_mark_end(sg_to);
> + return copied;
> +}
> +
> +/*
> + * Compares section of 'sg' starting from offset 'offset' and with length
> 'len'
> + * to linear buffer of length 'len' at address 'buffer'
The comment should document the return value.
> + */
> +static bool sg_compare_to_buffer(struct scatterlist *sg,
> + int offset, u8 *buffer, size_t len)
> +{
> + unsigned long flags;
> + int retval = 0;
> + struct sg_mapping_iter miter;
> +
> + local_irq_save(flags);
hm, the "IRQ disabled if SG_MITER_ATOMIC" requirement of
sg_miter_next() is weird and unexpected.
Tejun's 137d3edb48425f8 added the code whch has this strange
requirement, but it is unexplained. Wazzup?
> + sg_miter_start(, sg, sg_nents(sg),
> + SG_MITER_ATOMIC | SG_MITER_FROM_SG);
> +
> + while (sg_miter_next() && len > 0) {
> +
> + int cmplen;
> +
> + if (offset >= miter.length) {
> + offset -= miter.length;
> + continue;
> + }
> +
> + cmplen = min(miter.length - offset, len);
> + retval = memcmp(miter.addr + offset, buffer, cmplen);
> + if (retval)
> + break;
> +
> + buffer += cmplen;
> + len -= cmplen;
> + offset = 0;
> + }
> +
> + if (!retval && len)
> + retval = -1;
> +
> + sg_miter_stop();
> + local_irq_restore(flags);
> + return retval;
> +}
>
> ...
>
> +static void msb_mark_block_unused(struct msb_data *msb, int pba)
> +{
> + int zone = msb_get_zone_from_pba(pba);
> +
> + if (!test_bit(pba, msb->used_blocks_bitmap)) {
> + ms_printk("BUG: attempt to mark "
> + "already unused pba %d as unused" , pba);
> + msb->read_only = true;
> + return;
> + }
> +
> +#ifdef DEBUG
> + if
Re: [PATCH] memstick: add support for legacy memorysticks
On Wed, 19 Sep 2012 16:19:02 +0300
Maxim Levitsky maximlevit...@gmail.com wrote:
Based partially on MS standard spec quotes from Alex Dubov.
As any code that works with user data this driver isn't
recommended to use to write cards that contain valuable data.
It tries its best though to avoid data corruption
and possible damage to the card.
Tested on MS DUO 64 MB card on Ricoh R592 card reader..
Generally looks nice to me, although I wouldn't know a memstick if one
stuck to my shoe.
I have a bunch of fairly trivial comments, and one head-scratcher for
Tejun: what's up with the local-irqs-disabled requirement in sg_miter_next()?
...
+static int sg_nents(struct scatterlist *sg)
+{
+ int nents = 0;
+ while (sg) {
+ nents++;
+ sg = sg_next(sg);
+ }
+
+ return nents;
+}
I think we may as well put this in scatterlist.c immediately. It
wouldn't surprise me if we already have open-coded copies of this lying
around the tree.
+/*
+ * Copies section of 'sg_from' starting from offset 'offset' and with length
+ * 'len' To another scatterlist of to_nents enties
+ */
+static int sg_copy(struct scatterlist *sg_from, struct scatterlist *sg_to,
+ int to_nents, int offset, size_t len)
The should return a size_t, to match the type of `len'. And the type
of `offset' is fishy - there's nothing which intrinsically limits these
things to 2G?
+{
+ int copied = 0;
+
+ while (offset 0) {
+
+ if (offset = sg_from-length) {
+ if (sg_is_last(sg_from))
+ return 0;
+
+ offset -= sg_from-length;
+ sg_from = sg_next(sg_from);
+ continue;
+ }
+
+ copied = min(len, (size_t)(sg_from-length - offset));
min_t would be more conventional. Or perhaps even min(), depending on
`offset's new type (but probably not).
+ sg_set_page(sg_to, sg_page(sg_from),
+ copied, sg_from-offset + offset);
+
+ len -= copied;
+ offset = 0;
+
+ if (sg_is_last(sg_from) || !len)
+ goto out;
+
+ sg_to = sg_next(sg_to);
+ to_nents--;
+ sg_from = sg_next(sg_from);
+ }
+
+ while (len sg_from-length to_nents--) {
+
+ len -= sg_from-length;
+ copied += sg_from-length;
+
+ sg_set_page(sg_to, sg_page(sg_from),
+ sg_from-length, sg_from-offset);
+
+ if (sg_is_last(sg_from) || !len)
+ goto out;
+
+ sg_from = sg_next(sg_from);
+ sg_to = sg_next(sg_to);
+ }
+
+ if (len to_nents) {
+ sg_set_page(sg_to, sg_page(sg_from), len, sg_from-offset);
+ copied += len;
+ }
+
+out:
+ sg_mark_end(sg_to);
+ return copied;
+}
+
+/*
+ * Compares section of 'sg' starting from offset 'offset' and with length
'len'
+ * to linear buffer of length 'len' at address 'buffer'
The comment should document the return value.
+ */
+static bool sg_compare_to_buffer(struct scatterlist *sg,
+ int offset, u8 *buffer, size_t len)
+{
+ unsigned long flags;
+ int retval = 0;
+ struct sg_mapping_iter miter;
+
+ local_irq_save(flags);
hm, the IRQ disabled if SG_MITER_ATOMIC requirement of
sg_miter_next() is weird and unexpected.
Tejun's 137d3edb48425f8 added the code whch has this strange
requirement, but it is unexplained. Wazzup?
+ sg_miter_start(miter, sg, sg_nents(sg),
+ SG_MITER_ATOMIC | SG_MITER_FROM_SG);
+
+ while (sg_miter_next(miter) len 0) {
+
+ int cmplen;
+
+ if (offset = miter.length) {
+ offset -= miter.length;
+ continue;
+ }
+
+ cmplen = min(miter.length - offset, len);
+ retval = memcmp(miter.addr + offset, buffer, cmplen);
+ if (retval)
+ break;
+
+ buffer += cmplen;
+ len -= cmplen;
+ offset = 0;
+ }
+
+ if (!retval len)
+ retval = -1;
+
+ sg_miter_stop(miter);
+ local_irq_restore(flags);
+ return retval;
+}
...
+static void msb_mark_block_unused(struct msb_data *msb, int pba)
+{
+ int zone = msb_get_zone_from_pba(pba);
+
+ if (!test_bit(pba, msb-used_blocks_bitmap)) {
+ ms_printk(BUG: attempt to mark
+ already unused pba %d as unused , pba);
+ msb-read_only = true;
+ return;
+ }
+
+#ifdef DEBUG
+ if (msb_validate_used_block_bitmap(msb))
+ return;
+#endif
+ clear_bit(pba, msb-used_blocks_bitmap);
+ msb-free_block_count[zone]++;
Re: [PATCH] memstick: add support for legacy memorysticks
On Wed, 2012-09-19 at 14:52 -0700, Andrew Morton wrote:
On Wed, 19 Sep 2012 16:19:02 +0300
Maxim Levitsky maximlevit...@gmail.com wrote:
Based partially on MS standard spec quotes from Alex Dubov.
As any code that works with user data this driver isn't
recommended to use to write cards that contain valuable data.
It tries its best though to avoid data corruption
and possible damage to the card.
Tested on MS DUO 64 MB card on Ricoh R592 card reader..
Generally looks nice to me, although I wouldn't know a memstick if one
stuck to my shoe.
I have a bunch of fairly trivial comments, and one head-scratcher for
Tejun: what's up with the local-irqs-disabled requirement in sg_miter_next()?
Awesome, thanks for the review!
I will address all comments very soon.
...
+static int sg_nents(struct scatterlist *sg)
+{
+ int nents = 0;
+ while (sg) {
+ nents++;
+ sg = sg_next(sg);
+ }
+
+ return nents;
+}
I think we may as well put this in scatterlist.c immediately. It
wouldn't surprise me if we already have open-coded copies of this lying
around the tree.
I agree, but as usual afraid that it wont be accepted here.
+/*
+ * Copies section of 'sg_from' starting from offset 'offset' and with
length
+ * 'len' To another scatterlist of to_nents enties
+ */
+static int sg_copy(struct scatterlist *sg_from, struct scatterlist *sg_to,
+ int to_nents, int offset, size_t len)
The should return a size_t, to match the type of `len'. And the type
of `offset' is fishy - there's nothing which intrinsically limits these
things to 2G?
In my case, I will never have large offsets here, but I agree with you
completely.
+{
+ int copied = 0;
+
+ while (offset 0) {
+
+ if (offset = sg_from-length) {
+ if (sg_is_last(sg_from))
+ return 0;
+
+ offset -= sg_from-length;
+ sg_from = sg_next(sg_from);
+ continue;
+ }
+
+ copied = min(len, (size_t)(sg_from-length - offset));
min_t would be more conventional. Or perhaps even min(), depending on
`offset's new type (but probably not).
Agree.
+ sg_set_page(sg_to, sg_page(sg_from),
+ copied, sg_from-offset + offset);
+
+ len -= copied;
+ offset = 0;
+
+ if (sg_is_last(sg_from) || !len)
+ goto out;
+
+ sg_to = sg_next(sg_to);
+ to_nents--;
+ sg_from = sg_next(sg_from);
+ }
+
+ while (len sg_from-length to_nents--) {
+
+ len -= sg_from-length;
+ copied += sg_from-length;
+
+ sg_set_page(sg_to, sg_page(sg_from),
+ sg_from-length, sg_from-offset);
+
+ if (sg_is_last(sg_from) || !len)
+ goto out;
+
+ sg_from = sg_next(sg_from);
+ sg_to = sg_next(sg_to);
+ }
+
+ if (len to_nents) {
+ sg_set_page(sg_to, sg_page(sg_from), len, sg_from-offset);
+ copied += len;
+ }
+
+out:
+ sg_mark_end(sg_to);
+ return copied;
+}
+
+/*
+ * Compares section of 'sg' starting from offset 'offset' and with length
'len'
+ * to linear buffer of length 'len' at address 'buffer'
The comment should document the return value.
+ */
+static bool sg_compare_to_buffer(struct scatterlist *sg,
+ int offset, u8 *buffer, size_t len)
+{
+ unsigned long flags;
+ int retval = 0;
+ struct sg_mapping_iter miter;
+
+ local_irq_save(flags);
hm, the IRQ disabled if SG_MITER_ATOMIC requirement of
sg_miter_next() is weird and unexpected.
Tejun's 137d3edb48425f8 added the code whch has this strange
requirement, but it is unexplained. Wazzup?
+ sg_miter_start(miter, sg, sg_nents(sg),
+ SG_MITER_ATOMIC | SG_MITER_FROM_SG);
+
+ while (sg_miter_next(miter) len 0) {
+
+ int cmplen;
+
+ if (offset = miter.length) {
+ offset -= miter.length;
+ continue;
+ }
+
+ cmplen = min(miter.length - offset, len);
+ retval = memcmp(miter.addr + offset, buffer, cmplen);
+ if (retval)
+ break;
+
+ buffer += cmplen;
+ len -= cmplen;
+ offset = 0;
+ }
+
+ if (!retval len)
+ retval = -1;
+
+ sg_miter_stop(miter);
+ local_irq_restore(flags);
+ return retval;
+}
...
+static void msb_mark_block_unused(struct msb_data *msb, int pba)
+{
+ int zone = msb_get_zone_from_pba(pba);
+
+ if (!test_bit(pba, msb-used_blocks_bitmap)) {
+ ms_printk(BUG: attempt to mark
+
