libata PM question

libata PM question

[Benjamin Herrenschmidt]

Hi there !

So I (finally !) resumed work on pata_macio.c to replace ide/pmac.c :-)

I've started looking at the suspend/resume stuff. It looks like most of
it just comes for free thanks to libata, yay ! :-)

One quick nit tho, in libata-eh.c, in ata_eh_handle_port_suspend(), any
reason why you always call ata_acpi_set_state(ap, PMSG_SUSPEND) rather
than ata_acpi_set_state(ap, ap->pm_mesg) ?

Not directly related to my pmac driver but it seems like it would cause
calls into ACPI suspend/resume code even when doing freeze/thaw for
hibernation which doesn't sound totally right to me ... or do I miss
something ?

Cheers,
Ben.

Re: libata PM question

[Benjamin Herrenschmidt]

On Mon, 2009-11-30 at 13:22 +1100, Benjamin Herrenschmidt wrote:
> Hi there !
> 
> So I (finally !) resumed work on pata_macio.c to replace ide/pmac.c :-)
> 
> I've started looking at the suspend/resume stuff. It looks like most of
> it just comes for free thanks to libata, yay ! :-)

 .../...

Oh, another question... where do we spin the disk down & park it on
suspend ? I'm missed at least that part :-)

Cheers,
Ben

Re: libata PM question

[Benjamin Herrenschmidt]

On Mon, 2009-11-30 at 13:25 +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2009-11-30 at 13:22 +1100, Benjamin Herrenschmidt wrote:
> > Hi there !
> > 
> > So I (finally !) resumed work on pata_macio.c to replace ide/pmac.c :-)
> > 
> > I've started looking at the suspend/resume stuff. It looks like most of
> > it just comes for free thanks to libata, yay ! :-)
> 
>  .../...
> 
> Oh, another question... where do we spin the disk down & park it on
> suspend ? I'm missed at least that part :-)

Oh and finally, one last question for today ...

In drivers/ide, we have a call to ledtrig_ide_activity() inside
ide_do_rw_disk() which allows me to flash the front light on
the powerbook.

libata has no such thing.

It's as simple as adding that call, if we want to keep the trigger named
"ide-disk" but where to put it then ? In ata_scsi_rw_xlat() ? Yuck :-)

Another option is to stick it in drivers/scsi/sd.c but the trigger
should not be named ide-disk anymore ... Also it will start tiggering
for any "pseudo scsi" device in the machine which is note quite the
purpose... It would be nice if it really only triggered on libata and as
such blink the led only for the internal HD on those powerbooks (unless
you have some libata based PCMCIA stuff but that doesn't matter).

Anyway, nothing fancy, just a quick hack to get the activity led
back :-) I could always stick it somewhere in pata_macio.c but that
would suck probably even more.

Cheers,
Ben.
 

Re: libata PM question

[Robert Hancock]

On 11/29/2009 08:25 PM, Benjamin Herrenschmidt wrote:
> On Mon, 2009-11-30 at 13:22 +1100, Benjamin Herrenschmidt wrote:
>> Hi there !
>>
>> So I (finally !) resumed work on pata_macio.c to replace ide/pmac.c :-)
>>
>> I've started looking at the suspend/resume stuff. It looks like most of
>> it just comes for free thanks to libata, yay ! :-)
>
>   .../...
>
> Oh, another question... where do we spin the disk down&  park it on
> suspend ? I'm missed at least that part :-)

sd driver handles that part (it issues a START STOP UNIT command which 
gets translated into STANDBY IMMEDIATE).

Re: libata PM question

[Robert Hancock]

On 11/29/2009 09:42 PM, Benjamin Herrenschmidt wrote:
> On Mon, 2009-11-30 at 13:25 +1100, Benjamin Herrenschmidt wrote:
>> On Mon, 2009-11-30 at 13:22 +1100, Benjamin Herrenschmidt wrote:
>>> Hi there !
>>>
>>> So I (finally !) resumed work on pata_macio.c to replace ide/pmac.c :-)
>>>
>>> I've started looking at the suspend/resume stuff. It looks like most of
>>> it just comes for free thanks to libata, yay ! :-)
>>
>>   .../...
>>
>> Oh, another question... where do we spin the disk down&  park it on
>> suspend ? I'm missed at least that part :-)
>
> Oh and finally, one last question for today ...
>
> In drivers/ide, we have a call to ledtrig_ide_activity() inside
> ide_do_rw_disk() which allows me to flash the front light on
> the powerbook.
>
> libata has no such thing.
>
> It's as simple as adding that call, if we want to keep the trigger named
> "ide-disk" but where to put it then ? In ata_scsi_rw_xlat() ? Yuck :-)
>
> Another option is to stick it in drivers/scsi/sd.c but the trigger
> should not be named ide-disk anymore ... Also it will start tiggering
> for any "pseudo scsi" device in the machine which is note quite the
> purpose... It would be nice if it really only triggered on libata and as
> such blink the led only for the internal HD on those powerbooks (unless
> you have some libata based PCMCIA stuff but that doesn't matter).
>
> Anyway, nothing fancy, just a quick hack to get the activity led
> back :-) I could always stick it somewhere in pata_macio.c but that
> would suck probably even more.

Looks like the only libata driver that currently does LED blinking is 
ahci (for external enclosures) and it's just handled there using 
qc_issue plus a timer. Most drivers don't need this and given that you 
need to do it only for specific devices and only your driver has that 
knowledge, I don't see much problem with just sticking the LED trigger 
call into your qc_issue or something similar. No sense in trying to make 
something generic if it's not generic..

Re: libata PM question

[Benjamin Herrenschmidt]

> Looks like the only libata driver that currently does LED blinking is 
> ahci (for external enclosures) and it's just handled there using 
> qc_issue plus a timer. Most drivers don't need this and given that you 
> need to do it only for specific devices and only your driver has that 
> knowledge, I don't see much problem with just sticking the LED trigger 
> call into your qc_issue or something similar. No sense in trying to make 
> something generic if it's not generic..

Ok, thanks. Will stick it in pata_macio for the internal hard disk(s)
only.

Cheers,
Ben.

Re: libata PM question

[Tejun Heo]

(Shaohua Li cc'd)
Hello,

On 11/30/2009 11:22 AM, Benjamin Herrenschmidt wrote:
> One quick nit tho, in libata-eh.c, in ata_eh_handle_port_suspend(), any
> reason why you always call ata_acpi_set_state(ap, PMSG_SUSPEND) rather
> than ata_acpi_set_state(ap, ap->pm_mesg) ?
> 
> Not directly related to my pmac driver but it seems like it would cause
> calls into ACPI suspend/resume code even when doing freeze/thaw for
> hibernation which doesn't sound totally right to me ... or do I miss
> something ?

Indeed, it looks like it will power down the IDE port over freeze.
Shaohua, this is added by commit
bd3adca52bc43b72c75db3e4c7809d47923b154c.  Skipping power down on
freeze is probably the right thing to do?

Thanks.

-- 
tejun

Re: libata PM question

[Shaohua Li]

On Mon, 2009-11-30 at 15:26 +0800, Tejun Heo wrote:
> (Shaohua Li cc'd)
> Hello,
> 
> On 11/30/2009 11:22 AM, Benjamin Herrenschmidt wrote:
> > One quick nit tho, in libata-eh.c, in ata_eh_handle_port_suspend(), any
> > reason why you always call ata_acpi_set_state(ap, PMSG_SUSPEND) rather
> > than ata_acpi_set_state(ap, ap->pm_mesg) ?
> > 
> > Not directly related to my pmac driver but it seems like it would cause
> > calls into ACPI suspend/resume code even when doing freeze/thaw for
> > hibernation which doesn't sound totally right to me ... or do I miss
> > something ?
> 
> Indeed, it looks like it will power down the IDE port over freeze.
> Shaohua, this is added by commit
> bd3adca52bc43b72c75db3e4c7809d47923b154c.  Skipping power down on
> freeze is probably the right thing to do?
yes, looks reasonable though spec doesn't mention such case.

Thanks,
Shaohua

Re: libata PM question

[Tejun Heo]

On 11/30/2009 04:32 PM, Shaohua Li wrote:
> yes, looks reasonable though spec doesn't mention such case.

Freezing being a software thing, I think that's understandable.  Can
you please post a patch to change that?

Thanks.

-- 
tejun