* Re: [PATCH 2.6.9-rc2-mm1] Add missing del_timer_sync in ub_disconnect (was Re: [2.6.9-rc2-mm1] BUG at kernel/timer.c:414) [u]
[not found] ` <20040923130359.072f5109@lembas.zaitcev.lan>
@ 2004-09-23 20:22 ` Martin Schlemmer [c]
2004-09-24 0:09 ` Martin Schlemmer [c]
1 sibling, 0 replies; 2+ messages in thread
From: Martin Schlemmer [c] @ 2004-09-23 20:22 UTC (permalink / raw)
To: Pete Zaitcev; +Cc: Sahara Workshop, Andrew Morton, Linux Kernel Mailing Lists
[-- Attachment #1: Type: text/plain, Size: 7929 bytes --]
On Thu, 2004-09-23 at 13:03 -0700, Pete Zaitcev wrote:
> On Thu, 23 Sep 2004 21:23:33 +0200
> Sahara Workshop <workshop@cpt.sahara.co.za> wrote:
>
> > Wild chance I am taking, but what about this (not tested):
>
> > @@ -849,6 +849,7 @@ static void ub_scsi_action(unsigned long
> > spin_lock_irqsave(&sc->lock, flags);
> > ub_scsi_dispatch(sc);
> > spin_unlock_irqrestore(&sc->lock, flags);
> > + del_timer_sync(&sc->work_timer);
> > }
>
> I'm not too sure about using del_timer_sync here. It does not sleep
> overtly, so it should be safe. But why bother. Please try the
> attached.
>
Will do thanks. I am totally in the dark in reference to urb/whatever
api, so was not sure if it could be before the
tasklet was scheduled (in other words - I was not sure you
could do it in the callback itself .. prob on droids at the
time or something =) ...
> -- Pete
>
> P.S. Can you do anything about those offensive disclaimers?
>
Not much from work :/ Tomorrow is public holiday though, and
I am for a change at home the whole of the weekend, so if this
do not take longer than the weekend you should not see it
again :-)
Thanks,
Martin
> diff -urp -X dontdiff linux-2.6.9-rc2-mm1/drivers/block/ub.c linux-2.6.9-rc2-mm1-ub/drivers/block/ub.c
> --- linux-2.6.9-rc2-mm1/drivers/block/ub.c 2004-09-17 23:04:27.000000000 -0700
> +++ linux-2.6.9-rc2-mm1-ub/drivers/block/ub.c 2004-09-23 10:39:03.415548736 -0700
> @@ -25,6 +25,7 @@
> * -- prune comments, they are too volumnous
> * -- Exterminate P3 printks
> * -- Resove XXX's
> + * -- Redo "benh's retries", perhaps have spin-up code to handle them. V:D=?
> */
> #include <linux/kernel.h>
> #include <linux/module.h>
> @@ -157,7 +158,8 @@ struct ub_scsi_cmd {
> struct ub_scsi_cmd *next;
>
> int error; /* Return code - valid upon done */
> - int act_len; /* Return size */
> + unsigned int act_len; /* Return size */
> + unsigned char key, asc, ascq; /* May be valid if error==-EIO */
>
> int stat_count; /* Retries getting status. */
>
> @@ -673,9 +675,12 @@ static inline int ub_bd_rq_fn_1(request_
>
> /*
> * build the command
> + *
> + * The call to blk_queue_hardsect_size() guarantees that request
> + * is aligned, but it is given in terms of 512 byte units, always.
> */
> - block = rq->sector;
> - nblks = rq->nr_sectors;
> + block = rq->sector >> sc->capacity.bshift;
> + nblks = rq->nr_sectors >> sc->capacity.bshift;
>
> memset(cmd, 0, sizeof(struct ub_scsi_cmd));
> cmd->cdb[0] = (ub_dir == UB_DIR_READ)? READ_10: WRITE_10;
> @@ -690,7 +695,7 @@ static inline int ub_bd_rq_fn_1(request_
> cmd->dir = ub_dir;
> cmd->state = UB_CMDST_INIT;
> cmd->data = rq->buffer;
> - cmd->len = nblks * 512;
> + cmd->len = rq->nr_sectors * 512;
> cmd->done = ub_rw_cmd_done;
> cmd->back = rq;
>
> @@ -837,6 +842,7 @@ static void ub_urb_complete(struct urb *
> {
> struct ub_dev *sc = urb->context;
>
> + del_timer(&sc->work_timer);
> ub_complete(&sc->work_done);
> tasklet_schedule(&sc->tasklet);
> }
> @@ -1141,16 +1147,8 @@ static void ub_scsi_urb_compl(struct ub_
> (*cmd->done)(sc, cmd);
>
> } else if (cmd->state == UB_CMDST_SENSE) {
> - /*
> - * We do not look at sense, because even if there was no sense,
> - * we get into UB_CMDST_SENSE from a STALL or CSW FAIL only.
> - * We request sense because we want to clear CHECK CONDITION
> - * on devices with delusions of SCSI, and not because we
> - * are curious in any way about the sense itself.
> - */
> - /* if ((cmd->top_sense[2] & 0x0F) == NO_SENSE) { foo } */
> -
> ub_state_done(sc, cmd, -EIO);
> +
> } else {
> printk(KERN_WARNING "%s: "
> "wrong command state %d on device %u\n",
> @@ -1309,6 +1307,10 @@ static void ub_top_sense_done(struct ub_
> */
> ub_cmdtr_sense(sc, scmd, sense);
>
> + /*
> + * Find the command which triggered the unit attention or a check,
> + * save the sense into it, and advance its state machine.
> + */
> if ((cmd = ub_cmdq_peek(sc)) == NULL) {
> printk(KERN_WARNING "%s: sense done while idle\n", sc->name);
> return;
> @@ -1326,6 +1328,10 @@ static void ub_top_sense_done(struct ub_
> return;
> }
>
> + cmd->key = sense[2] & 0x0F;
> + cmd->asc = sense[12];
> + cmd->ascq = sense[13];
> +
> ub_scsi_urb_compl(sc, cmd);
> }
>
> @@ -1377,6 +1383,18 @@ static void ub_revalidate(struct ub_dev
> * XXX sd.c sets capacity to zero in such case. However, it doesn't
> * work for us. In case of zero capacity, block layer refuses to
> * have the /dev/uba opened (why?) Set capacity to some random value.
> +
> +(4/14/2004 about 2.6.9-rc1-mm4)
> +<jejb> viro: actually because there was a check on size == 0 before assigning f_ops so you can never open a zero size device to revalidate it
> +<viro> jejb: ah, I remember
> +<jejb> viro: I'm still seeing on scsi that I need to issue two BLKRRPARTs to get the partition table read
> +<viro> jejb: there was a very odd API for issuing commands on absent device
> +<viro> jejb: IIRC, cciss, ida or DAC960
> +<zaitcev> jejb: I had that problem with a zero size device which I "solved" by setting the size to some random number (50KB).
> +<hch> viro: DAC960 allowed to issue ioctls when opened with O_NONBLOCK
> +* viro really ought to dig out the 2.7 projects and do some triage
> +<jejb> zaitcev: yes, we do that in SCSI. However, putting bogus sizes in is rather silly
> +
> */
> sc->capacity.nsec = 50;
> sc->capacity.bsize = 512;
> @@ -1519,7 +1537,7 @@ static int ub_bd_revalidate(struct gendi
> sc->name, sc->dev->devnum, sc->capacity.nsec, sc->capacity.bsize);
>
> /* XXX Support sector size switching like in sr.c */
> - // blk_queue_hardsect_size(q, sc->capacity.bsize);
> + blk_queue_hardsect_size(disk->queue, sc->capacity.bsize);
> set_capacity(disk, sc->capacity.nsec);
> // set_disk_ro(sdkp->disk, sc->readonly);
>
> @@ -1621,6 +1639,9 @@ static int ub_sync_tur(struct ub_dev *sc
>
> rc = cmd->error;
>
> + if (rc == -EIO && cmd->key != 0) /* Retries for benh's key */
> + rc = cmd->key;
> +
> err_submit:
> kfree(cmd);
> err_alloc:
> @@ -1836,6 +1857,7 @@ static int ub_probe(struct usb_interface
> request_queue_t *q;
> struct gendisk *disk;
> int rc;
> + int i;
>
> rc = -ENOMEM;
> if ((sc = kmalloc(sizeof(struct ub_dev), GFP_KERNEL)) == NULL)
> @@ -1902,7 +1924,11 @@ static int ub_probe(struct usb_interface
> * has to succeed, so we clear checks with an additional one here.
> * In any case it's not our business how revaliadation is implemented.
> */
> - ub_sync_tur(sc);
> + for (i = 0; i < 3; i++) { /* Retries for benh's key */
> + if ((rc = ub_sync_tur(sc)) <= 0) break;
> + if (rc != 0x6) break;
> + msleep(10);
> + }
>
> sc->removable = 1; /* XXX Query this from the device */
>
> @@ -1938,7 +1964,7 @@ static int ub_probe(struct usb_interface
> blk_queue_max_phys_segments(q, UB_MAX_REQ_SG);
> // blk_queue_segment_boundary(q, CARM_SG_BOUNDARY);
> blk_queue_max_sectors(q, UB_MAX_SECTORS);
> - // blk_queue_hardsect_size(q, xxxxx);
> + blk_queue_hardsect_size(q, sc->capacity.bsize);
>
> /*
> * This is a serious infraction, caused by a deficiency in the
> @@ -2047,6 +2073,13 @@ static void ub_disconnect(struct usb_int
> spin_unlock_irqrestore(&sc->lock, flags);
>
> /*
> + * There is virtually no chance that other CPU runs times so long
> + * after ub_urb_complete should have called del_timer, but only if HCD
> + * didn't forget to deliver a callback on unlink.
> + */
> + del_timer_sync(&sc->work_timer);
> +
> + /*
> * At this point there must be no commands coming from anyone
> * and no URBs left in transit.
> */
--
Martin Schlemmer
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: [PATCH 2.6.9-rc2-mm1] Add missing del_timer_sync in ub_disconnect (was Re: [2.6.9-rc2-mm1] BUG at kernel/timer.c:414) [u]
[not found] ` <20040923130359.072f5109@lembas.zaitcev.lan>
2004-09-23 20:22 ` [PATCH 2.6.9-rc2-mm1] Add missing del_timer_sync in ub_disconnect (was Re: [2.6.9-rc2-mm1] BUG at kernel/timer.c:414) [u] Martin Schlemmer [c]
@ 2004-09-24 0:09 ` Martin Schlemmer [c]
1 sibling, 0 replies; 2+ messages in thread
From: Martin Schlemmer [c] @ 2004-09-24 0:09 UTC (permalink / raw)
To: Pete Zaitcev; +Cc: Sahara Workshop, Andrew Morton, Linux Kernel Mailing Lists
[-- Attachment #1: Type: text/plain, Size: 7580 bytes --]
On Thu, 2004-09-23 at 13:03 -0700, Pete Zaitcev wrote:
> On Thu, 23 Sep 2004 21:23:33 +0200
> Sahara Workshop <workshop@cpt.sahara.co.za> wrote:
>
> > Wild chance I am taking, but what about this (not tested):
>
> > @@ -849,6 +849,7 @@ static void ub_scsi_action(unsigned long
> > spin_lock_irqsave(&sc->lock, flags);
> > ub_scsi_dispatch(sc);
> > spin_unlock_irqrestore(&sc->lock, flags);
> > + del_timer_sync(&sc->work_timer);
> > }
>
> I'm not too sure about using del_timer_sync here. It does not sleep
> overtly, so it should be safe. But why bother. Please try the
> attached.
>
> -- Pete
>
> P.S. Can you do anything about those offensive disclaimers?
>
> diff -urp -X dontdiff linux-2.6.9-rc2-mm1/drivers/block/ub.c linux-2.6.9-rc2-mm1-ub/drivers/block/ub.c
> --- linux-2.6.9-rc2-mm1/drivers/block/ub.c 2004-09-17 23:04:27.000000000 -0700
> +++ linux-2.6.9-rc2-mm1-ub/drivers/block/ub.c 2004-09-23 10:39:03.415548736 -0700
> @@ -25,6 +25,7 @@
> * -- prune comments, they are too volumnous
> * -- Exterminate P3 printks
> * -- Resove XXX's
> + * -- Redo "benh's retries", perhaps have spin-up code to handle them. V:D=?
> */
> #include <linux/kernel.h>
> #include <linux/module.h>
> @@ -157,7 +158,8 @@ struct ub_scsi_cmd {
> struct ub_scsi_cmd *next;
>
> int error; /* Return code - valid upon done */
> - int act_len; /* Return size */
> + unsigned int act_len; /* Return size */
> + unsigned char key, asc, ascq; /* May be valid if error==-EIO */
>
> int stat_count; /* Retries getting status. */
>
> @@ -673,9 +675,12 @@ static inline int ub_bd_rq_fn_1(request_
>
> /*
> * build the command
> + *
> + * The call to blk_queue_hardsect_size() guarantees that request
> + * is aligned, but it is given in terms of 512 byte units, always.
> */
> - block = rq->sector;
> - nblks = rq->nr_sectors;
> + block = rq->sector >> sc->capacity.bshift;
> + nblks = rq->nr_sectors >> sc->capacity.bshift;
>
> memset(cmd, 0, sizeof(struct ub_scsi_cmd));
> cmd->cdb[0] = (ub_dir == UB_DIR_READ)? READ_10: WRITE_10;
> @@ -690,7 +695,7 @@ static inline int ub_bd_rq_fn_1(request_
> cmd->dir = ub_dir;
> cmd->state = UB_CMDST_INIT;
> cmd->data = rq->buffer;
> - cmd->len = nblks * 512;
> + cmd->len = rq->nr_sectors * 512;
> cmd->done = ub_rw_cmd_done;
> cmd->back = rq;
>
> @@ -837,6 +842,7 @@ static void ub_urb_complete(struct urb *
> {
> struct ub_dev *sc = urb->context;
>
> + del_timer(&sc->work_timer);
> ub_complete(&sc->work_done);
> tasklet_schedule(&sc->tasklet);
> }
> @@ -1141,16 +1147,8 @@ static void ub_scsi_urb_compl(struct ub_
> (*cmd->done)(sc, cmd);
>
> } else if (cmd->state == UB_CMDST_SENSE) {
> - /*
> - * We do not look at sense, because even if there was no sense,
> - * we get into UB_CMDST_SENSE from a STALL or CSW FAIL only.
> - * We request sense because we want to clear CHECK CONDITION
> - * on devices with delusions of SCSI, and not because we
> - * are curious in any way about the sense itself.
> - */
> - /* if ((cmd->top_sense[2] & 0x0F) == NO_SENSE) { foo } */
> -
> ub_state_done(sc, cmd, -EIO);
> +
> } else {
> printk(KERN_WARNING "%s: "
> "wrong command state %d on device %u\n",
> @@ -1309,6 +1307,10 @@ static void ub_top_sense_done(struct ub_
> */
> ub_cmdtr_sense(sc, scmd, sense);
>
> + /*
> + * Find the command which triggered the unit attention or a check,
> + * save the sense into it, and advance its state machine.
> + */
> if ((cmd = ub_cmdq_peek(sc)) == NULL) {
> printk(KERN_WARNING "%s: sense done while idle\n", sc->name);
> return;
> @@ -1326,6 +1328,10 @@ static void ub_top_sense_done(struct ub_
> return;
> }
>
> + cmd->key = sense[2] & 0x0F;
> + cmd->asc = sense[12];
> + cmd->ascq = sense[13];
> +
> ub_scsi_urb_compl(sc, cmd);
> }
>
> @@ -1377,6 +1383,18 @@ static void ub_revalidate(struct ub_dev
> * XXX sd.c sets capacity to zero in such case. However, it doesn't
> * work for us. In case of zero capacity, block layer refuses to
> * have the /dev/uba opened (why?) Set capacity to some random value.
> +
> +(4/14/2004 about 2.6.9-rc1-mm4)
> +<jejb> viro: actually because there was a check on size == 0 before assigning f_ops so you can never open a zero size device to revalidate it
> +<viro> jejb: ah, I remember
> +<jejb> viro: I'm still seeing on scsi that I need to issue two BLKRRPARTs to get the partition table read
> +<viro> jejb: there was a very odd API for issuing commands on absent device
> +<viro> jejb: IIRC, cciss, ida or DAC960
> +<zaitcev> jejb: I had that problem with a zero size device which I "solved" by setting the size to some random number (50KB).
> +<hch> viro: DAC960 allowed to issue ioctls when opened with O_NONBLOCK
> +* viro really ought to dig out the 2.7 projects and do some triage
> +<jejb> zaitcev: yes, we do that in SCSI. However, putting bogus sizes in is rather silly
> +
> */
> sc->capacity.nsec = 50;
> sc->capacity.bsize = 512;
> @@ -1519,7 +1537,7 @@ static int ub_bd_revalidate(struct gendi
> sc->name, sc->dev->devnum, sc->capacity.nsec, sc->capacity.bsize);
>
> /* XXX Support sector size switching like in sr.c */
> - // blk_queue_hardsect_size(q, sc->capacity.bsize);
> + blk_queue_hardsect_size(disk->queue, sc->capacity.bsize);
> set_capacity(disk, sc->capacity.nsec);
> // set_disk_ro(sdkp->disk, sc->readonly);
>
> @@ -1621,6 +1639,9 @@ static int ub_sync_tur(struct ub_dev *sc
>
> rc = cmd->error;
>
> + if (rc == -EIO && cmd->key != 0) /* Retries for benh's key */
> + rc = cmd->key;
> +
> err_submit:
> kfree(cmd);
> err_alloc:
> @@ -1836,6 +1857,7 @@ static int ub_probe(struct usb_interface
> request_queue_t *q;
> struct gendisk *disk;
> int rc;
> + int i;
>
> rc = -ENOMEM;
> if ((sc = kmalloc(sizeof(struct ub_dev), GFP_KERNEL)) == NULL)
> @@ -1902,7 +1924,11 @@ static int ub_probe(struct usb_interface
> * has to succeed, so we clear checks with an additional one here.
> * In any case it's not our business how revaliadation is implemented.
> */
> - ub_sync_tur(sc);
> + for (i = 0; i < 3; i++) { /* Retries for benh's key */
> + if ((rc = ub_sync_tur(sc)) <= 0) break;
> + if (rc != 0x6) break;
> + msleep(10);
> + }
>
> sc->removable = 1; /* XXX Query this from the device */
>
> @@ -1938,7 +1964,7 @@ static int ub_probe(struct usb_interface
> blk_queue_max_phys_segments(q, UB_MAX_REQ_SG);
> // blk_queue_segment_boundary(q, CARM_SG_BOUNDARY);
> blk_queue_max_sectors(q, UB_MAX_SECTORS);
> - // blk_queue_hardsect_size(q, xxxxx);
> + blk_queue_hardsect_size(q, sc->capacity.bsize);
>
> /*
> * This is a serious infraction, caused by a deficiency in the
> @@ -2047,6 +2073,13 @@ static void ub_disconnect(struct usb_int
> spin_unlock_irqrestore(&sc->lock, flags);
>
> /*
> + * There is virtually no chance that other CPU runs times so long
> + * after ub_urb_complete should have called del_timer, but only if HCD
> + * didn't forget to deliver a callback on unlink.
> + */
> + del_timer_sync(&sc->work_timer);
> +
> + /*
> * At this point there must be no commands coming from anyone
> * and no URBs left in transit.
> */
Seems to work fine here at home. I will fiddle with it some more over
the weekend, but will only be back at work on Monday.
Thanks,
--
Martin Schlemmer
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 2+ messages in thread