libata hotplug question

libata hotplug question

[Benjamin Herrenschmidt]

So pata_macio is starting to look good, it even suspends and resumes on
a couple of test laptops, now is time to sort out the last piece of the
puzzle, which is the hotplug media-bay.

The old code use to call directly into drivers/ide ide_port_scan()
etc... from within the mediabay driver. Pretty filthy.

I'm changing that to something that's even simpler: the macio_driver
gets a new callback for plug/unplug events from the bay, so it will be
easy to keep the old driver do whatever drivers/ide cruft it wants
locally and do something different in libata.

Now, for libata, I haven't totally figured out what to do though.

It seems like when the state "changes", I can do something like ahci and
call ata_ehi_hotplugged() followed by something like ata_port_freeze()
to kick the EH... at least that's my rough understanding.

But I don't quite get how to inform libata that the part has or has not
something plugged in it. I thought about playing with the probe_mask but
it looks like ata_eh_link_autopsy() will reset that since I'm PATA, not
SATA and thus have no sata_scr_read()...

Any suggestion here ?

Cheers,
Ben.

Re: libata hotplug question

[Benjamin Herrenschmidt]

On Mon, 2009-11-30 at 17:04 +1100, Benjamin Herrenschmidt wrote:
> So pata_macio is starting to look good, it even suspends and resumes on
> a couple of test laptops, now is time to sort out the last piece of the
> puzzle, which is the hotplug media-bay.
> 
> The old code use to call directly into drivers/ide ide_port_scan()
> etc... from within the mediabay driver. Pretty filthy.
> 
> I'm changing that to something that's even simpler: the macio_driver
> gets a new callback for plug/unplug events from the bay, so it will be
> easy to keep the old driver do whatever drivers/ide cruft it wants
> locally and do something different in libata.
> 
> Now, for libata, I haven't totally figured out what to do though.
> 
> It seems like when the state "changes", I can do something like ahci and
> call ata_ehi_hotplugged() followed by something like ata_port_freeze()
> to kick the EH... at least that's my rough understanding.

One idea that comes to mind (I will try hacking something later today)
is to make ata_link_online() and ata_link_offline() use optional hooks
in the port ops so my driver can replace them instead of using the SATA
PHY stuff. Would that fly ?

Cheers,
Ben.

Re: libata hotplug question

[Tejun Heo]

Hello,

On 11/30/2009 03:04 PM, Benjamin Herrenschmidt wrote:
> Now, for libata, I haven't totally figured out what to do though.
> 
> It seems like when the state "changes", I can do something like ahci and
> call ata_ehi_hotplugged() followed by something like ata_port_freeze()
> to kick the EH... at least that's my rough understanding.

Yeap, that should be enough.

> But I don't quite get how to inform libata that the part has or has not
> something plugged in it. I thought about playing with the probe_mask but
> it looks like ata_eh_link_autopsy() will reset that since I'm PATA, not
> SATA and thus have no sata_scr_read()...

No, it won't reset the mask.  It will only reset if SCR read failed
with errors other than -EOPNOTSUPP.  If your driver isn't implementing
SCR access, it will fail with -EOPNOTSUPP.  Also, if SCR access fails,
autopsy dosen't clear probe_mask, it sets all bits there forcing
recovery part to full probing.

Anyways, setting probe_mask (if you don't know which is gone, setting
all bits works fine) and calling ata_port_freeze() is enough.  The
actual hot plug/unlpug path is the same as boot probing path, so
there's nothing more to do there.

Thanks.

-- 
tejun

Re: libata hotplug question

[Tejun Heo]

Hello,

On 12/01/2009 08:44 AM, Benjamin Herrenschmidt wrote:
> On Mon, 2009-11-30 at 17:04 +1100, Benjamin Herrenschmidt wrote:
>> So pata_macio is starting to look good, it even suspends and resumes on
>> a couple of test laptops, now is time to sort out the last piece of the
>> puzzle, which is the hotplug media-bay.
>>
>> The old code use to call directly into drivers/ide ide_port_scan()
>> etc... from within the mediabay driver. Pretty filthy.
>>
>> I'm changing that to something that's even simpler: the macio_driver
>> gets a new callback for plug/unplug events from the bay, so it will be
>> easy to keep the old driver do whatever drivers/ide cruft it wants
>> locally and do something different in libata.
>>
>> Now, for libata, I haven't totally figured out what to do though.
>>
>> It seems like when the state "changes", I can do something like ahci and
>> call ata_ehi_hotplugged() followed by something like ata_port_freeze()
>> to kick the EH... at least that's my rough understanding.
> 
> One idea that comes to mind (I will try hacking something later today)
> is to make ata_link_online() and ata_link_offline() use optional hooks
> in the port ops so my driver can replace them instead of using the SATA
> PHY stuff. Would that fly ?

Oh... yeah, that was the original intention when adding those
functions but you wouldn't need them for hot plug/unplug.  Just set
probe mask and freeze the port.  EH will do the right thing.

-- 
tejun

Re: libata hotplug question

[Benjamin Herrenschmidt]

On Tue, 2009-12-01 at 08:48 +0900, Tejun Heo wrote:
> > One idea that comes to mind (I will try hacking something later
> today)
> > is to make ata_link_online() and ata_link_offline() use optional
> hooks
> > in the port ops so my driver can replace them instead of using the
> SATA
> > PHY stuff. Would that fly ?
> 
> Oh... yeah, that was the original intention when adding those
> functions but you wouldn't need them for hot plug/unplug.  Just set
> probe mask and freeze the port.  EH will do the right thing. 

Ok. I'll try that. Thanks.

Cheers,
Ben.

Re: libata hotplug question

[Benjamin Herrenschmidt]

> Oh... yeah, that was the original intention when adding those
> functions but you wouldn't need them for hot plug/unplug.  Just set
> probe mask and freeze the port.  EH will do the right thing.

So I experimented a bit... and not touching probe_mask at all, just
doing ata_ehi_hotplugged and ata_port_freeze seems to get the basics
working, however with a few issues.

For example, if I remove the device, it will spend a few second trying
to poke at it & reset the bus before deciding its gone. This isn't a
terribly good idea (and could cause "interesting" issues if it's
re-plugged right away in fact)

I tried toying with the probe_mask bits but it doesn't seem to make any
difference here. I must admit that I got more or less lost as to what
probe_mask actually means, how it's used, when it's initialized and when
it's cleared. I will try to spend some more time later looking at the
code and trying to sort it out but it doesn't appear to be a great
solution.

At the end of the day, it might be better to just add those link
callbacks... I'll give that a try too, unless you have a better idea.

In addition, to speed up boot time, I want to remove the synchronous
waiting for the media bay to be up that we have in there, which is easy
since it's already run by a kthread.

The reason I had that was originally to provide some stability in drive
ordering with old drivers/ide and have the drive ready at boot when the
driver was probed, but that isn't really necessary anymore. I will
provide a callback to keep doing that synchronous wait for the old IDE
driver because I don't want to deal with races there at boot time, but
for libata, I can just trigger the EH for a hotplug at about any time.

But for that to work right, I need to be able to prevent probing when
setting up the host, when I know that the bay is empty, which the link
stuff would cover nicely too, don't you think ? Or do you have a better
idea ?

I also have to be careful of a possible race if the hotplug callback
since it can potentially happen as soon as I enter my driver->probe()
(it comes from the macio "bus" layer). What is the harm of triggering a
hotplug if nothing was actually unplugged ? Will libata silently just
keep things as they were ?

Cheers,
Ben. 

Re: libata hotplug question

[Tejun Heo]

Hello, Benjamin.

On 12/01/2009 11:43 AM, Benjamin Herrenschmidt wrote:
> 
>> Oh... yeah, that was the original intention when adding those
>> functions but you wouldn't need them for hot plug/unplug.  Just set
>> probe mask and freeze the port.  EH will do the right thing.
> 
> So I experimented a bit... and not touching probe_mask at all, just
> doing ata_ehi_hotplugged and ata_port_freeze seems to get the basics
> working, however with a few issues.
> 
> For example, if I remove the device, it will spend a few second trying
> to poke at it & reset the bus before deciding its gone. This isn't a
> terribly good idea (and could cause "interesting" issues if it's
> re-plugged right away in fact)

This is actually required for SATA at least.  Because there usually
isn't a separate mechanism to determine device hotplugginga, any link
layer glitch can't be discerned from hotplug events and in SATA link
glitches happens from time to time even on perfectly healthy systems,
so if EH ditches the device immediately after phy event, the system
will lose filesystems underneath it quite often, so the grace period
(IIRCa it's around 15secs) is intentional.

If someone removes power to a drive and then reapplies it while it's
working, then there just isn't so much libata can do about that.  If
ATA devices had a event counter or a flag which indicates that it lost
data in the buffer, then libata would be able to detect such events
and ditch the device immediately and reprobe it as a different device.
That would also solve the data loss issues with flaky power supplies
but without such mechanism, we just can't discern between the two.

If the hotplug events are completely reliable, we can add a flag to
tell libata EH to trust the hotplug notices completely but then again
I'm not too convinced about the benefits of implementing it in very
small number of drivers.  It could be more beneficial to keep the
behavior consistent across all ATA drivers.

> I tried toying with the probe_mask bits but it doesn't seem to make any
> difference here. I must admit that I got more or less lost as to what
> probe_mask actually means, how it's used, when it's initialized and when
> it's cleared. I will try to spend some more time later looking at the
> code and trying to sort it out but it doesn't appear to be a great
> solution.

It's basically an advisory flag telling libata to try to look for new
devices.  From outside EH, the only behavior difference is that with
the flag set EH will try to re-enable ports which got disabled due to
repeated reset failures.

> At the end of the day, it might be better to just add those link
> callbacks... I'll give that a try too, unless you have a better idea.

Link callbacks won't change the hotplug behavior tho.

> In addition, to speed up boot time, I want to remove the synchronous
> waiting for the media bay to be up that we have in there, which is easy
> since it's already run by a kthread.
> 
> The reason I had that was originally to provide some stability in drive
> ordering with old drivers/ide and have the drive ready at boot when the
> driver was probed, but that isn't really necessary anymore. I will
> provide a callback to keep doing that synchronous wait for the old IDE
> driver because I don't want to deal with races there at boot time, but
> for libata, I can just trigger the EH for a hotplug at about any time.
> 
> But for that to work right, I need to be able to prevent probing when
> setting up the host, when I know that the bay is empty, which the link
> stuff would cover nicely too, don't you think ? Or do you have a better
> idea ?

Oh yeah, link stuff will help that.  Or you can also implement custom
softreset which checks link status and exits early if there's no
device.  The only problem with the latter approach is that you'll need
to take care of the race window in the driver itself.  You can close
the window by checking link status again in postreset and rescheduling
hotplug event if hotplug event happened after the link state is
checked in softreset but before the port was thawed.

> I also have to be careful of a possible race if the hotplug callback
> since it can potentially happen as soon as I enter my driver->probe()
> (it comes from the macio "bus" layer). What is the harm of triggering a
> hotplug if nothing was actually unplugged ? Will libata silently just
> keep things as they were ?

Those events are all advisory.  The final decision is always upto what
the reset methods and following probes tell the EH, so it may go
through one more EH round but it won't lose anything.

Thanks.

-- 
tejun

Re: libata hotplug question

[Benjamin Herrenschmidt]

On Tue, 2009-12-01 at 13:43 +1100, Benjamin Herrenschmidt wrote:
> > Oh... yeah, that was the original intention when adding those
> > functions but you wouldn't need them for hot plug/unplug.  Just set
> > probe mask and freeze the port.  EH will do the right thing.

 .../...

> But for that to work right, I need to be able to prevent probing when
> setting up the host, when I know that the bay is empty, which the link
> stuff would cover nicely too, don't you think ? Or do you have a better
> idea ?
> 
> I also have to be careful of a possible race if the hotplug callback
> since it can potentially happen as soon as I enter my driver->probe()
> (it comes from the macio "bus" layer). What is the harm of triggering a
> hotplug if nothing was actually unplugged ? Will libata silently just
> keep things as they were ?

So I tried with implementing the link_online and link_offline callbacks
and it doesn't fly very high.

When I unplug the bay, it displays:

ata3: exception Emask 0x10 SAct 0x0 SErr 0x0 action 0xe frozen
ata3: EH complete

and doesn't really "unplug" it (ie, trying to access the device then
causes all sorts of errors to pile up in the log). Plugging it back then
causes it to soft reset and restore the DMA timings and it works again.

But it's overall sub optimal. Comparatively, it worked better without
the link callbacks though I would call it sub-optimal :-)

What would be the best way to tell libata to just remove the device and
not bother poking the HW again on an unplug and do as it does today on a
plug without the link callbacks ?

Cheers,
Ben.

Re: libata hotplug question

[Tejun Heo]

Hello,

On 12/01/2009 02:17 PM, Benjamin Herrenschmidt wrote:
> So I tried with implementing the link_online and link_offline callbacks
> and it doesn't fly very high.
> 
> When I unplug the bay, it displays:
> 
> ata3: exception Emask 0x10 SAct 0x0 SErr 0x0 action 0xe frozen
> ata3: EH complete

You didn't schedule any EH actions, right?

> and doesn't really "unplug" it (ie, trying to access the device then
> causes all sorts of errors to pile up in the log). Plugging it back then
> causes it to soft reset and restore the DMA timings and it works again.
> 
> But it's overall sub optimal. Comparatively, it worked better without
> the link callbacks though I would call it sub-optimal :-)
> 
> What would be the best way to tell libata to just remove the device and
> not bother poking the HW again on an unplug and do as it does today on a
> plug without the link callbacks ?

libata-acpi.c does that.  Setting ATA_DFLAG_DETACH device flag and
scheduling EH will do that.  Please take a look at
ata_acpi_detach_device() but you must be *sure* that your hot unplug
notification is completely reliable.

Thanks.

-- 
tejun

Re: libata hotplug question

[Benjamin Herrenschmidt]

On Tue, 2009-12-01 at 13:51 +0900, Tejun Heo wrote:
> 
> If the hotplug events are completely reliable, we can add a flag to
> tell libata EH to trust the hotplug notices completely but then again
> I'm not too convinced about the benefits of implementing it in very
> small number of drivers.  It could be more beneficial to keep the
> behavior consistent across all ATA drivers.

Well... in my case I know for sure the device is out. So the stuff
libata does to try to poke at the device is not only a waste of time,
but a few scary messages in the dmesg log ;-)

> It's basically an advisory flag telling libata to try to look for new
> devices.  From outside EH, the only behavior difference is that with
> the flag set EH will try to re-enable ports which got disabled due to
> repeated reset failures.

Ok, but ata_ehi_hotplugged() will set it anyway so I don't really have
to do anything.

> > At the end of the day, it might be better to just add those link
> > callbacks... I'll give that a try too, unless you have a better
> idea.
> 
> Link callbacks won't change the hotplug behavior tho.

Right, see my other message, they make the situation even more complex
in fact so I'm backing off that.

> Oh yeah, link stuff will help that.  Or you can also implement custom
> softreset which checks link status and exits early if there's no
> device.

That might be an option, I'll look at it.

> The only problem with the latter approach is that you'll need
> to take care of the race window in the driver itself.  You can close
> the window by checking link status again in postreset and rescheduling
> hotplug event if hotplug event happened after the link state is
> checked in softreset but before the port was thawed.

I've done simpler. I've added a pair of calls to lock/unlock the media
bay which directly grab the bay driver internal mutex, which guarantees
we won't get any callback while we hold it. I could have done something
like you described instead but this is easy and allow to also easily
solve some of the possible races with drivers/ide while at it.

> Those events are all advisory.  The final decision is always upto what
> the reset methods and following probes tell the EH, so it may go
> through one more EH round but it won't lose anything.

That's what I'm starting to understand :-) Very good. I'll look at the
custom softreset approach.

Cheers,
Ben.

Re: libata hotplug question

[Benjamin Herrenschmidt]

On Tue, 2009-12-01 at 14:22 +0900, Tejun Heo wrote:
> Hello,
> 
> On 12/01/2009 02:17 PM, Benjamin Herrenschmidt wrote:
> > So I tried with implementing the link_online and link_offline callbacks
> > and it doesn't fly very high.
> > 
> > When I unplug the bay, it displays:
> > 
> > ata3: exception Emask 0x10 SAct 0x0 SErr 0x0 action 0xe frozen
> > ata3: EH complete
> 
> You didn't schedule any EH actions, right?

I did the usual ehi_hotplug + freeze

> libata-acpi.c does that.  Setting ATA_DFLAG_DETACH device flag and
> scheduling EH will do that.  Please take a look at
> ata_acpi_detach_device() but you must be *sure* that your hot unplug
> notification is completely reliable.

Well, it seems to be reliable that way for unplugs. I even debounce it
both ways but I'm actually considering removing the debounce on unplug.

I'll let you know how it goes with a custom prereset() that returns
-ENODEV. How would that differ from ATA_DFLAG_DETACH ?

Cheers,
Ben.

Re: libata hotplug question

[Tejun Heo]

On 12/01/2009 02:30 PM, Benjamin Herrenschmidt wrote:
>> You didn't schedule any EH actions, right?
> 
> I did the usual ehi_hotplug + freeze

Hmmm.... then I have no idea what happened there.  :-(

>> libata-acpi.c does that.  Setting ATA_DFLAG_DETACH device flag and
>> scheduling EH will do that.  Please take a look at
>> ata_acpi_detach_device() but you must be *sure* that your hot unplug
>> notification is completely reliable.
> 
> Well, it seems to be reliable that way for unplugs. I even debounce it
> both ways but I'm actually considering removing the debounce on unplug.
> 
> I'll let you know how it goes with a custom prereset() that returns
> -ENODEV. How would that differ from ATA_DFLAG_DETACH ?

In effect, they would be the same but if the hotplug notification can
be made reliable, I think using DFLAG_DETACH would be better as
there's already other users doing that (user requested detach and ACPI
dock removal).

Thanks.

-- 
tejun

Re: libata hotplug question

[Benjamin Herrenschmidt]

On Tue, 2009-12-01 at 14:22 +0900, Tejun Heo wrote:
> libata-acpi.c does that.  Setting ATA_DFLAG_DETACH device flag and
> scheduling EH will do that.  Please take a look at
> ata_acpi_detach_device() but you must be *sure* that your hot unplug
> notification is completely reliable.

That looks even better than prereset hook actually... I'll have a look

Cheers
Ben.

Re: libata hotplug question

[Benjamin Herrenschmidt]

On Tue, 2009-12-01 at 14:34 +0900, Tejun Heo wrote:
> In effect, they would be the same but if the hotplug notification can
> be made reliable, I think using DFLAG_DETACH would be better as
> there's already other users doing that (user requested detach and ACPI
> dock removal). 

Allright. I just need to find how to tell it at boot that it's detached
to avoid time probing a non existing interface but that's no big deal.

Cheers,
Ben.

Re: libata hotplug question

[Benjamin Herrenschmidt]

On Tue, 2009-12-01 at 16:39 +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2009-12-01 at 14:34 +0900, Tejun Heo wrote:
> > In effect, they would be the same but if the hotplug notification can
> > be made reliable, I think using DFLAG_DETACH would be better as
> > there's already other users doing that (user requested detach and ACPI
> > dock removal). 
> 
> Allright. I just need to find how to tell it at boot that it's detached
> to avoid time probing a non existing interface but that's no big deal.

No point bothering, it looks like it picks up the ff's at boot just fine
like an unpopulated interface and things just work without any spurrious
delay.

I'll do a few more tests and will send the patches for review.

Note that I had to do one change to libata-sff:

--- linux-work.orig/drivers/ata/libata-sff.c	2009-12-01 16:51:10.000000000 +1100
+++ linux-work/drivers/ata/libata-sff.c	2009-12-01 16:52:59.000000000 +1100
@@ -2384,7 +2384,7 @@ void ata_sff_post_internal_cmd(struct at
 	ap->hsm_task_state = HSM_ST_IDLE;
 
 	if (ap->ioaddr.bmdma_addr)
-		ata_bmdma_stop(qc);
+		ap->ops->bmdma_stop(qc);
 
 	spin_unlock_irqrestore(ap->lock, flags);

I'll include that change in my patch series.

Cheers,
Ben.