Re: [PATCH] IDE TCQ #4

Re: [PATCH] IDE TCQ #4

[Petr Vandrovec]

On 15 Apr 02 at 21:11, Petr Vandrovec wrote:
> On 15 Apr 02 at 21:00, Jens Axboe wrote:
> > > 
> > > NULL pointer ...
> > 
> > Could you decode that? It doesn't look like any of your drives support
> > TCQ, it should have enabled them right here:
> 
> They were already decoded... Also others reported that - after accessing
> /proc/ide/ide0/hda/identify system dies... I believe that passing
> hand-created request to ide_raw_taskfile corrupts drive->free_req,
> and so subsequent drive command after this cat finds that 
> drive->free_req.next is NULL and dies.

ide_raw_taskfile() sets rq.special to &ar - and &ar is on the stack,
in this function. Later it falls through to __ide_end_request(), which
does

  ar = rq->special;
  ...
  if (ar)
    ata_ar_put(drive, ar);
    
which adds this ar into drive's free_req chain unconditionally. Maybe 
ata_ar_put should check for ar_queue validity. And where ar_queue
member is initialized (or at least cleared) in this case at all?

Unfortunately here my knowledge ends.
                                                Petr Vandrovec
                                                vandrove@vc.cvut.cz
                                                

Re: [PATCH] IDE TCQ #4

[Jens Axboe]

On Mon, Apr 15 2002, Petr Vandrovec wrote:
> On 15 Apr 02 at 21:11, Petr Vandrovec wrote:
> > On 15 Apr 02 at 21:00, Jens Axboe wrote:
> > > > 
> > > > NULL pointer ...
> > > 
> > > Could you decode that? It doesn't look like any of your drives support
> > > TCQ, it should have enabled them right here:
> > 
> > They were already decoded... Also others reported that - after accessing
> > /proc/ide/ide0/hda/identify system dies... I believe that passing
> > hand-created request to ide_raw_taskfile corrupts drive->free_req,
> > and so subsequent drive command after this cat finds that 
> > drive->free_req.next is NULL and dies.
> 
> ide_raw_taskfile() sets rq.special to &ar - and &ar is on the stack,
> in this function. Later it falls through to __ide_end_request(), which
> does
> 
>   ar = rq->special;
>   ...
>   if (ar)
>     ata_ar_put(drive, ar);
>     
> which adds this ar into drive's free_req chain unconditionally. Maybe 
> ata_ar_put should check for ar_queue validity. And where ar_queue
> member is initialized (or at least cleared) in this case at all?

yes this looks like a silly problem. the fix should be to have
ata_ar_get() set ATA_AR_RETURN in ar_flags:

        if (!list_empty(&drive->free_req)) {
                ar = list_ata_entry(drive->free_req.next);
                list_del(&ar->ar_queue);
                ata_ar_init(drive, ar);
                ar->ar_flags |= ATA_AR_RETURN;
        }

and then only have ata_ar_put() readd it to the list when it is set:

static inline void ata_ar_put(ide_drive_t *drive, struct ata_request
*ar)
{
        if (ar->ar_flags & ATA_AR_RETURN)
                list_add(&ar->ar_queue, &drive->free_req);
	...

Then you can also remove the ata_ar_put() conditional in
ide_end_drive_cmd(), just call ata_ar_put() unconditionally.

> Unfortunately here my knowledge ends.

it was very helpful :-)

-- 
Jens Axboe

USB-Mouse-Bug in 2.4.16-8 ?

[mtopper]

Dear Mailing List, 

I've discovered that even if I insmod (or modprobe) the proper USB modules
for my 2.4.16 kernel, and if I use the USB mouse afterwards,
"lsmod" reveals that the modules seem to be "(0) unused" - despite the USB
mouse is in action!

Users of the 2.4.18-kernel affirmed same kernel behaviour.

If I rmmod the USB modules, they subsequently allow to be removed from
kernelspace, and the USB mouse cursor , ofcourse , stops instantly -
despite he was just in use. 

Is this a bug or a feature? :-)

Yours, Alex

Re: USB-Mouse-Bug in 2.4.16-8 ?

[Greg KH]

On Tue, Apr 16, 2002 at 12:39:07PM +0200, mtopper@xarch.tu-graz.ac.at wrote:
> 
> Dear Mailing List, 
> 
> I've discovered that even if I insmod (or modprobe) the proper USB modules
> for my 2.4.16 kernel, and if I use the USB mouse afterwards,
> "lsmod" reveals that the modules seem to be "(0) unused" - despite the USB
> mouse is in action!
> 
> Users of the 2.4.18-kernel affirmed same kernel behaviour.
> 
> If I rmmod the USB modules, they subsequently allow to be removed from
> kernelspace, and the USB mouse cursor , ofcourse , stops instantly -
> despite he was just in use. 
> 
> Is this a bug or a feature? :-)

This is the way the module was designed, so yes it's a feature :)

thanks,

greg k-h

Re: [PATCH] IDE TCQ #4

[Martin Dalecki]

Jens Axboe wrote:


> yes this looks like a silly problem. the fix should be to have
> ata_ar_get() set ATA_AR_RETURN in ar_flags:
> 
>         if (!list_empty(&drive->free_req)) {
>                 ar = list_ata_entry(drive->free_req.next);
>                 list_del(&ar->ar_queue);
>                 ata_ar_init(drive, ar);
>                 ar->ar_flags |= ATA_AR_RETURN;
>         }
> 
> and then only have ata_ar_put() readd it to the list when it is set:
> 
> static inline void ata_ar_put(ide_drive_t *drive, struct ata_request
> *ar)
> {
>         if (ar->ar_flags & ATA_AR_RETURN)
>                 list_add(&ar->ar_queue, &drive->free_req);
> 	...
> 
> Then you can also remove the ata_ar_put() conditional in
> ide_end_drive_cmd(), just call ata_ar_put() unconditionally.

Well something similar is already in IDE 37... I have just
invented a flag ATA_AR_STATIC which get's set in ide_raw_taskfile
ata_ar_put ich then checking for if (!(ar->ar_flags & ATA_AR_STATIC))...

It has the desired effect in practice.

Re: [PATCH] IDE TCQ #4

[Jens Axboe]

On Tue, Apr 16 2002, Martin Dalecki wrote:
> Jens Axboe wrote:
> 
> 
> >yes this looks like a silly problem. the fix should be to have
> >ata_ar_get() set ATA_AR_RETURN in ar_flags:
> >
> >        if (!list_empty(&drive->free_req)) {
> >                ar = list_ata_entry(drive->free_req.next);
> >                list_del(&ar->ar_queue);
> >                ata_ar_init(drive, ar);
> >                ar->ar_flags |= ATA_AR_RETURN;
> >        }
> >
> >and then only have ata_ar_put() readd it to the list when it is set:
> >
> >static inline void ata_ar_put(ide_drive_t *drive, struct ata_request
> >*ar)
> >{
> >        if (ar->ar_flags & ATA_AR_RETURN)
> >                list_add(&ar->ar_queue, &drive->free_req);
> >	...
> >
> >Then you can also remove the ata_ar_put() conditional in
> >ide_end_drive_cmd(), just call ata_ar_put() unconditionally.
> 
> Well something similar is already in IDE 37... I have just
> invented a flag ATA_AR_STATIC which get's set in ide_raw_taskfile
> ata_ar_put ich then checking for if (!(ar->ar_flags & ATA_AR_STATIC))...
> 
> It has the desired effect in practice.

sure, just used ATA_AR_RETURN since it was there already. I'm not
particularly fond of that name though, and ATA_AR_STATIC isn't too good
either imo. how about ATA_AR_POOL? with the same semantics as
ATA_AR_RETURN, ie return to pool if flag is set.

-- 
Jens Axboe

Re: [PATCH] IDE TCQ #4

[Martin Dalecki]

Jens Axboe wrote:
> On Tue, Apr 16 2002, Martin Dalecki wrote:
> 
>>Jens Axboe wrote:
>>
>>
>>
>>>yes this looks like a silly problem. the fix should be to have
>>>ata_ar_get() set ATA_AR_RETURN in ar_flags:
>>>
>>>       if (!list_empty(&drive->free_req)) {
>>>               ar = list_ata_entry(drive->free_req.next);
>>>               list_del(&ar->ar_queue);
>>>               ata_ar_init(drive, ar);
>>>               ar->ar_flags |= ATA_AR_RETURN;
>>>       }
>>>
>>>and then only have ata_ar_put() readd it to the list when it is set:
>>>
>>>static inline void ata_ar_put(ide_drive_t *drive, struct ata_request
>>>*ar)
>>>{
>>>       if (ar->ar_flags & ATA_AR_RETURN)
>>>               list_add(&ar->ar_queue, &drive->free_req);
>>>	...
>>>
>>>Then you can also remove the ata_ar_put() conditional in
>>>ide_end_drive_cmd(), just call ata_ar_put() unconditionally.
>>
>>Well something similar is already in IDE 37... I have just
>>invented a flag ATA_AR_STATIC which get's set in ide_raw_taskfile
>>ata_ar_put ich then checking for if (!(ar->ar_flags & ATA_AR_STATIC))...
>>
>>It has the desired effect in practice.
> 
> 
> sure, just used ATA_AR_RETURN since it was there already. I'm not
> particularly fond of that name though, and ATA_AR_STATIC isn't too good
> either imo. how about ATA_AR_POOL? with the same semantics as
> ATA_AR_RETURN, ie return to pool if flag is set.

ATA_AR_POOL sounds good. It indicates where it's comming from and
where it is going to remain. BTW.> Have you noticed the tactile steps
forward I did in conversion of ide-cd.c to the new command submission in IDE 37?
Any suggestions? It isn't really that abvious how to get finally rid
of struct packt_command... but I think that I'm not far away now.
The only thing that has to be done is to move the sense data and failed
command used in ide-cd away from the packet command structure.
I will not pass it as pointer around but just copy it directly to struct
ata_devices driver_data field... I think.