ZPODD Patches

drivers/ata/libata-zpodd.c +15 -0

+void zpodd_init(struct ata_device *dev)
+{
+	int ret;
+	struct zpodd *zpodd;
+
+	if (dev->private_data)
+		return;
+
+	if (!device_can_poweroff(dev))
+		return;
+
+	if ((ret = check_loading_mechanism(dev)) == -ENODEV)
+		return;
+
+	zpodd = kzalloc(sizeof(struct zpodd), GFP_KERNEL);

Hello,

drivers/ata/libata-acpi.c void ata_acpi_bind(struct ata_device *dev) · line 1063

Quoted 4 lines · Aaron Lu

void ata_acpi_unbind(struct ata_device *dev)
{

drivers/ata/libata-acpi.c +2 -0

+	if (zpodd_dev_enabled(dev))
+		zpodd_deinit(dev);

Maybe zpodd_exit() instead?

Quoted 23 lines

drivers/ata/libata-zpodd.c +22 -0

+void zpodd_init(struct ata_device *dev)
+{
+	int ret;
+	struct zpodd *zpodd;
+
+	if (dev->private_data)
+		return;
+
+	if (!device_can_poweroff(dev))
+		return;
+
+	if ((ret = check_loading_mechanism(dev)) == -ENODEV)
+		return;
+
+	zpodd = kzalloc(sizeof(struct zpodd), GFP_KERNEL);
+	if (!zpodd)
+		return;
+
+	if (ret)
+		zpodd->drawer = true;
+	else
+		zpodd->slot = true;

drivers/ata/libata-zpodd.c line 117

Quoted 3 lines

drivers/ata/libata-zpodd.c +3 -0

+
+	zpodd->dev = dev;
+	dev->private_data = zpodd;

I don't think you're supposed to use dev->private_data from libata core layer. Just add a new field. Nobody cares about adding 8 more bytes to struct ata_device and spending 8 more bytes is way better than muddying the ownership of ->private_data.

drivers/ata/libata.h static inline void ata_sff_exit(void) · line 243

Quoted 12 lines

drivers/ata/libata.h +11 -0

+/* libata-zpodd.c */
+#ifdef CONFIG_SATA_ZPODD
+void zpodd_init(struct ata_device *dev);
+void zpodd_deinit(struct ata_device *dev);
+static inline bool zpodd_dev_enabled(struct ata_device *dev)
+{
+	if (dev->flags & ATA_DFLAG_DA && dev->private_data)
+		return true;
+	else
+		return false;
+}

And this gets completely wrong. What if the device supports DA and low level driver makes use of ->private_data?

Quoted 4 lines

I don't think you're supposed to use dev->private_data from libata core layer. Just add a new field. Nobody cares about adding 8 more bytes to struct ata_device and spending 8 more bytes is way better than muddying the ownership of ->private_data.

OK.
And just out of curiosity, who's supposed to use device's private_data? I didn't find any user for ata_device's private_data in libata.

Quoted 2 lines

And this gets completely wrong. What if the device supports DA and low level driver makes use of ->private_data?

I didn't find any user of ata_device's private_data, so I used it for ZPODD. But if this is dangerous, I'll use a new field.

Hello, Aaron.

Quoted 8 lines

Quoted 4 lines

I don't think you're supposed to use dev->private_data from libata core layer. Just add a new field. Nobody cares about adding 8 more bytes to struct ata_device and spending 8 more bytes is way better than muddying the ownership of ->private_data.

OK.
And just out of curiosity, who's supposed to use device's private_data? I didn't find any user for ata_device's private_data in libata.

All the ->private_data fields are to be used by low level drivers (ahci, ata_piix, pata_via...). Given the twisted nature of ATA devices, it's a bit surprising that no driver yet found a need for ata_dev->private_data. For most SATA controllers, port to device is one to one so maybe ap->private_data is enough.

Quoted 5 lines

Quoted 2 lines

And this gets completely wrong. What if the device supports DA and low level driver makes use of ->private_data?

I didn't find any user of ata_device's private_data, so I used it for ZPODD. But if this is dangerous, I'll use a new field.

As there currently is no other user, it won't break anything but yeah please add a properly typed and named field.

Thanks.

Since the ata acpi notification code introduced in commit 3bd46600a7a7e938c54df8cdbac9910668c7dfb0 is solely for ZPODD, and we now have a dedicated place for it, move these code there.

And the add/remove_pm_notifier code is simplified a little bit that it does not check things like if the handle is NULL and if a corresponding acpi_device is there for the handle as they are guaranteed by the device_can_poweroff already.

Quoted 4 lines · Aaron Lu

Since the ata acpi notification code introduced in commit 3bd46600a7a7e938c54df8cdbac9910668c7dfb0 is solely for ZPODD, and we now have a dedicated place for it, move these code there.

Quoted 4 lines

And the add/remove_pm_notifier code is simplified a little bit that it does not check things like if the handle is NULL and if a corresponding acpi_device is there for the handle as they are guaranteed by the device_can_poweroff already.

Please don't mix code movement with actual changes. It makes it difficult to track what's going on.

The atapi_eh_tur and atapi_eh_request_sense can be reused by ZPODD code, so separate them out to a file named libata-atapi.c, and the Makefile is modified accordingly. No functional changes should result from this commit.

Quoted 4 lines · Aaron Lu

The atapi_eh_tur and atapi_eh_request_sense can be reused by ZPODD code, so separate them out to a file named libata-atapi.c, and the Makefile is modified accordingly. No functional changes should result from this commit.

Why is this change necessary? This doesn't seem to help anything to me.

Thanks.

Signature · 1 line

tejun

Quoted 6 lines · Tejun Heo

Quoted 4 lines · Aaron Lu

The atapi_eh_tur and atapi_eh_request_sense can be reused by ZPODD code, so separate them out to a file named libata-atapi.c, and the Makefile is modified accordingly. No functional changes should result from this commit.

Quoted 2 lines

Why is this change necessary? This doesn't seem to help anything to me.

Function zpready introduced in patch 6 used these 2 functions to see if an ODD is in a zero power ready state.

Quoted 7 lines · Aaron Lu

Quoted 6 lines · Tejun Heo

Quoted 4 lines · Aaron Lu

The atapi_eh_tur and atapi_eh_request_sense can be reused by ZPODD code, so separate them out to a file named libata-atapi.c, and the Makefile is modified accordingly. No functional changes should result from this commit.

Quoted 5 lines

Quoted 2 lines

Why is this change necessary? This doesn't seem to help anything to me.

Function zpready introduced in patch 6 used these 2 functions to see if an ODD is in a zero power ready state.

And why zpready can't use the functions if they're in a different file?

My thought is that, the 2 functions are for ATAPI and can be used by libata-eh, libata-zpodd and maybe others in the future, so it deserves a separate place. But if this is regarded as unnecessary, I will drop this patch.

drivers/ata/libata-scsi.c +2 -0

+#define POWEROFF_DELAY  (30 * 1000)     /* 30 seconds for power off delay */
+
 struct zpodd {
 	bool slot:1;

+/*
+ * Snoop the result of GET_STATUS_NOTIFICATION_EVENT, the media
+ * status byte has information on media present/door closed.
+ *
+ * This information serves only as a hint, as it is not accurate.
+ * The sense code method will be used when deciding if the ODD is
+ * really zero power ready.
+ */

drivers/ata/libata-scsi.c +26 -0

+/*
+ * Check ODD's zero power ready status.
+ *
+ * This function is called during ATA port's suspend path,
+ * when the port is not frozen yet, so that we can still make
+ * some IO to the ODD to decide if it is zero power ready.
+ *
+ * The ODD is regarded as zero power ready when it is in zero
+ * power ready state for some time(defined by POWEROFF_DELAY).
+ */
+void zpodd_check_zpready(struct ata_device *dev)
+{
+	bool zp_ready;
+	unsigned long expires;
+	struct zpodd *zpodd = dev->private_data;
+
+	if (!zpodd->status_ready) {
+		zpodd->last_ready = 0;
+		return;
+	}
+
+	if (!zpodd->last_ready) {
+		zp_ready = zpready(dev);
+		if (zp_ready)
+			zpodd->last_ready = jiffies;
+		return;

Hello,

Quoted 30 lines · Aaron Lu

drivers/ata/libata-zpodd.c +30 -0

+/*
+ * Check ODD's zero power ready status.
+ *
+ * This function is called during ATA port's suspend path,
+ * when the port is not frozen yet, so that we can still make
+ * some IO to the ODD to decide if it is zero power ready.
+ *
+ * The ODD is regarded as zero power ready when it is in zero
+ * power ready state for some time(defined by POWEROFF_DELAY).
+ */
+void zpodd_check_zpready(struct ata_device *dev)
+{
+	bool zp_ready;
+	unsigned long expires;
+	struct zpodd *zpodd = dev->private_data;
+
+	if (!zpodd->status_ready) {
+		zpodd->last_ready = 0;
+		return;
+	}
+
+	if (!zpodd->last_ready) {
+		zp_ready = zpready(dev);
+		if (zp_ready)
+			zpodd->last_ready = jiffies;
+		return;
+	}
+
+	expires = zpodd->last_ready + msecs_to_jiffies(POWEROFF_DELAY);
+	if (time_before(jiffies, expires))

drivers/ata/libata-zpodd.c static bool device_can_poweroff(struct ata_device *ata_dev) · line 210

Quoted 6 lines

drivers/ata/libata-zpodd.c +6 -0

+		return;
+
+	zpodd->zp_ready = zpready(dev);
+	if (!zpodd->zp_ready)
+		zpodd->last_ready = 0;
+}

Hmmm... so, the "full" check only happens when autopm kicks in, right?

Is it really worth avoiding an extra TUR on autopm events? That's not really a hot path. It seems a bit over-engineered to me.

Hello,

Quoted 12 lines · Aaron Lu

drivers/ata/libata-zpodd.c +7 -0

+#define POWEROFF_DELAY  (30 * 1000)     /* 30 seconds for power off delay */
+
 struct zpodd {
 	bool slot:1;
 	bool drawer:1;
 	bool from_notify:1;	/* resumed as a result of acpi notification */
+	bool status_ready:1;	/* ready status derived from media event poll,
+				   it is not accurate, but serves as a hint */
+	bool zp_ready:1;	/* zero power ready state */
+
+	unsigned long last_ready; /* last zero power ready timestamp */

Quoted 2 lines

struct ata_device *dev;
};

How are accesses to the bit fields synchronized?

Quoted 9 lines

+/*
+ * Snoop the result of GET_STATUS_NOTIFICATION_EVENT, the media
+ * status byte has information on media present/door closed.
+ *
+ * This information serves only as a hint, as it is not accurate.
+ * The sense code method will be used when deciding if the ODD is
+ * really zero power ready.
+ */

It would be great if you can make the above a proper dockbook function comment. Also, as the snooping for hint thing isn't too obvious it would be great if the comment contains more info which is explained in the commit message.

Quoted 33 lines

drivers/ata/libata-zpodd.c +32 -0

+/*
+ * Check ODD's zero power ready status.
+ *
+ * This function is called during ATA port's suspend path,
+ * when the port is not frozen yet, so that we can still make
+ * some IO to the ODD to decide if it is zero power ready.
+ *
+ * The ODD is regarded as zero power ready when it is in zero
+ * power ready state for some time(defined by POWEROFF_DELAY).
+ */
+void zpodd_check_zpready(struct ata_device *dev)
+{
+	bool zp_ready;
+	unsigned long expires;
+	struct zpodd *zpodd = dev->private_data;
+
+	if (!zpodd->status_ready) {
+		zpodd->last_ready = 0;
+		return;
+	}
+
+	if (!zpodd->last_ready) {
+		zp_ready = zpready(dev);
+		if (zp_ready)
+			zpodd->last_ready = jiffies;
+		return;
+	}
+
+	expires = zpodd->last_ready + msecs_to_jiffies(POWEROFF_DELAY);
+	if (time_before(jiffies, expires))
+		return;
+

drivers/ata/libata-zpodd.c static bool device_can_poweroff(struct ata_device *ata_dev) · line 210

Quoted 4 lines

drivers/ata/libata-zpodd.c +4 -0

+	zpodd->zp_ready = zpready(dev);
+	if (!zpodd->zp_ready)
+		zpodd->last_ready = 0;
+}

Hmmm... so, the "full" check only happens when autopm kicks in, right? Is it really worth avoiding an extra TUR on autopm events? That's not really a hot path. It seems a bit over-engineered to me.

Quoted 15 lines · Tejun Heo

Hello,

Quoted 12 lines · Aaron Lu

drivers/ata/libata-zpodd.c +7 -0

+#define POWEROFF_DELAY  (30 * 1000)     /* 30 seconds for power off delay */
+
 struct zpodd {
 	bool slot:1;
 	bool drawer:1;
 	bool from_notify:1;	/* resumed as a result of acpi notification */
+	bool status_ready:1;	/* ready status derived from media event poll,
+				   it is not accurate, but serves as a hint */
+	bool zp_ready:1;	/* zero power ready state */
+
+	unsigned long last_ready; /* last zero power ready timestamp */

Quoted 4 lines

Quoted 2 lines

struct ata_device *dev;
};

How are accesses to the bit fields synchronized?

They are synchronized by PM core.
PM core ensures that no two suspend or resume callback run concurrently. And when ODD is executing a command, it is in active state, no PM callback will run.

Quoted 3 lines

Hmmm... so, the "full" check only happens when autopm kicks in, right? Is it really worth avoiding an extra TUR on autopm events? That's not really a hot path. It seems a bit over-engineered to me.

A little more information about this:
When there is disc inside and no program mounted the drive, the ODD will be runtime suspended/resumed every 2 seconds along with the event poll.

I'm not sure if the above situation can happen often. Normal desktop environment should automatically mount the ODD once they got uevent, and for console users, they will probably manually mount the drive after they have inserted a disc. The way I did it this way is to deal with the worst possible case. But if this is deemed as not necessary, I think I can remove the snoop hint thing and use TUR directly.

Hello, Aaron.

Quoted 21 lines · Tejun Heo

Quoted 15 lines

Hello,

Quoted 12 lines · Aaron Lu

drivers/ata/libata-zpodd.c +7 -0

+#define POWEROFF_DELAY  (30 * 1000)     /* 30 seconds for power off delay */
+
 struct zpodd {
 	bool slot:1;
 	bool drawer:1;
 	bool from_notify:1;	/* resumed as a result of acpi notification */
+	bool status_ready:1;	/* ready status derived from media event poll,
+				   it is not accurate, but serves as a hint */
+	bool zp_ready:1;	/* zero power ready state */
+
+	unsigned long last_ready; /* last zero power ready timestamp */

Quoted 4 lines

Quoted 2 lines

struct ata_device *dev;
};

How are accesses to the bit fields synchronized?

Quoted 4 lines

They are synchronized by PM core.
PM core ensures that no two suspend or resume callback run concurrently. And when ODD is executing a command, it is in active state, no PM callback will run.

Care to add short comment for that? Flag and bitfield updates aren't atomic to each other, so I find it usually helpful to clearly state the synchronization rules for them. More so if locking is inherited from upprer layer and not immediately obvious.

Quoted 8 lines

Quoted 4 lines

Hmmm... so, the "full" check only happens when autopm kicks in, right? Is it really worth avoiding an extra TUR on autopm events? That's not really a hot path. It seems a bit over-engineered to me.

A little more information about this:
When there is disc inside and no program mounted the drive, the ODD will be runtime suspended/resumed every 2 seconds along with the event poll.

Is that a desirable behavior? I haven't been following autopm and am a bit fuzzy about how autopm works and what it does.

If there isn't any user of the device autopm kicks in. If zpodd is enabled and there's no media, the device goes off power. If the user initiates an event which may change media status, the driver is notified via acpi and autopm backs out restoring power to the device. Am I understanding it correctly?

What I'm confused about is what autopm does for devices w/o zpodd. What happens then? Is it gonna leave power on for the device and, say, go on to suspend the controller? But, how would that work for, say, future devices with async notification for media events?

Also, if autopm is enabled, an optical device would go in and out of suspend every two seconds?

Quoted 6 lines

I'm not sure if the above situation can happen often. Normal desktop environment should automatically mount the ODD once they got uevent, and for console users, they will probably manually mount the drive after they have inserted a disc. The way I did it this way is to deal with the worst possible case. But if this is deemed as not necessary, I think I can remove the snoop hint thing and use TUR directly.

The problem with issuing TUR regularly is that some ODDs lock up after getting hit by frequent TURs. That's the reason why sr event check routine is being careful with TUR and only issue GET_EVENT_STATUS_NOTIFICATION. Windows does about the same thing and some vendors somehow screwed up TUR.

That said, I can't say the snooping is pretty. It's a rather nasty thing to do. So, libata now wants information from the event polling in block layer, but reaching for block_device from ata_devices is nasty too. Hmmm... but aren't you already doing that to block polling on a powered down device?

Quoted 4 lines

What I'm confused about is what autopm does for devices w/o zpodd. What happens then? Is it gonna leave power on for the device and, say, go on to suspend the controller? But, how would that work for, say, future devices with async notification for media events?

Maybe we shouldn't allow autopm for such devices?

Quoted 5 lines

That said, I can't say the snooping is pretty. It's a rather nasty thing to do. So, libata now wants information from the event polling in block layer, but reaching for block_device from ata_devices is nasty too. Hmmm... but aren't you already doing that to block polling on a powered down device?

I was feeling brain damaged by this for some time :-)

Basically, only ATA layer is aware of the power off thing, and sr knows nothing about this(or it is not supposed to know this, at least, this is what SCSI people think) and once powered off, I do not want the poll to disturb the device, so I need to block the poll. I can't come up with another way to achieve this except this nasty way.

James suggests me to keep the poll, but emulate the command. The problem with this is, the autopm for resume will kick in on each poll, so I'll need to decide if power up the ODD for this time's resume is needed in port's runtime resume callback. This made things complex and it also put too much logic in the resume callback, which is not desired. And even if I keep the ODD in powered off state by emulating this poll command, its ancestor devices will still be resumed, and I may need to do some trick in their resume callback to avoid needless power/state transitions. This doesn't feel like an elegant way to solve this either.

So yes, I'm still using this _nasty_ way to make the ODD stay in powered off state as long as possible. But if there is other elegant ways to solve this, I would appreciate it and happily using it. Personally, I hope we can make sr aware of ZPODD, that would make the pain gone.

Hey, Aaron.

Quoted 6 lines · Aaron Lu

Quoted 4 lines

What I'm confused about is what autopm does for devices w/o zpodd. What happens then? Is it gonna leave power on for the device and, say, go on to suspend the controller? But, how would that work for, say, future devices with async notification for media events?

Maybe we shouldn't allow autopm for such devices?

Yeah, maybe. It would be nice to be able to automatically power off disks and ports which aren't being used tho.

Quoted 24 lines

Quoted 5 lines

That said, I can't say the snooping is pretty. It's a rather nasty thing to do. So, libata now wants information from the event polling in block layer, but reaching for block_device from ata_devices is nasty too. Hmmm... but aren't you already doing that to block polling on a powered down device?

I was feeling brain damaged by this for some time :-)

Basically, only ATA layer is aware of the power off thing, and sr knows nothing about this(or it is not supposed to know this, at least, this is what SCSI people think) and once powered off, I do not want the poll to disturb the device, so I need to block the poll. I can't come up with another way to achieve this except this nasty way.

James suggests me to keep the poll, but emulate the command. The problem with this is, the autopm for resume will kick in on each poll, so I'll need to decide if power up the ODD for this time's resume is needed in port's runtime resume callback. This made things complex and it also put too much logic in the resume callback, which is not desired. And even if I keep the ODD in powered off state by emulating this poll command, its ancestor devices will still be resumed, and I may need to do some trick in their resume callback to avoid needless power/state transitions. This doesn't feel like an elegant way to solve this either.

Quoted 4 lines

So yes, I'm still using this _nasty_ way to make the ODD stay in powered off state as long as possible. But if there is other elegant ways to solve this, I would appreciate it and happily using it. Personally, I hope we can make sr aware of ZPODD, that would make the pain gone.

I really think we need a way for (auto)pm and event polling to talk to each other so that autopm can tell event poll to sod off while pm is in effect. Trying to solve this from inside libata doesn't seem right. The problem, again, seems to be figuring out which hardware device maps to which block device. Hmmm... Any good ideas?

Quoted 42 lines · Tejun Heo

Hey, Aaron.

Quoted 6 lines · Aaron Lu

Quoted 4 lines

What I'm confused about is what autopm does for devices w/o zpodd. What happens then? Is it gonna leave power on for the device and, say, go on to suspend the controller? But, how would that work for, say, future devices with async notification for media events?

Maybe we shouldn't allow autopm for such devices?

Yeah, maybe. It would be nice to be able to automatically power off disks and ports which aren't being used tho.

Quoted 24 lines

Quoted 5 lines

That said, I can't say the snooping is pretty. It's a rather nasty thing to do. So, libata now wants information from the event polling in block layer, but reaching for block_device from ata_devices is nasty too. Hmmm... but aren't you already doing that to block polling on a powered down device?

I was feeling brain damaged by this for some time :-)

Basically, only ATA layer is aware of the power off thing, and sr knows nothing about this(or it is not supposed to know this, at least, this is what SCSI people think) and once powered off, I do not want the poll to disturb the device, so I need to block the poll. I can't come up with another way to achieve this except this nasty way.

James suggests me to keep the poll, but emulate the command. The problem with this is, the autopm for resume will kick in on each poll, so I'll need to decide if power up the ODD for this time's resume is needed in port's runtime resume callback. This made things complex and it also put too much logic in the resume callback, which is not desired. And even if I keep the ODD in powered off state by emulating this poll command, its ancestor devices will still be resumed, and I may need to do some trick in their resume callback to avoid needless power/state transitions. This doesn't feel like an elegant way to solve this either.

Quoted 4 lines

So yes, I'm still using this _nasty_ way to make the ODD stay in powered off state as long as possible. But if there is other elegant ways to solve this, I would appreciate it and happily using it. Personally, I hope we can make sr aware of ZPODD, that would make the pain gone.

Quoted 5 lines

I really think we need a way for (auto)pm and event polling to talk to each other so that autopm can tell event poll to sod off while pm is in effect. Trying to solve this from inside libata doesn't seem right. The problem, again, seems to be figuring out which hardware device maps to which block device. Hmmm... Any good ideas?

I've asked the PM people several times about this, because it's a real problem for almost everything: PM needs some type of top to bottom stack view, which the layering isolation we have within storage really doesn't cope with well. No real suggestion has been forthcoming.

The reason I think it should be emulated (in the acpi layer, not libata, but as long as it's not in SCSI, I'm not so fussed where it ends up) is because ZPODD is the software equivalent of ZPREADY, which will be done in hardware and will be effectively invisible to autopm in the same way that SCSI (and ATA) power management is mostly invisible. If we currently do ZPREADY emulation in the low layer (i.e. ZPODD has exact ZPREADY emulation), we won't care (except for flipping the sofware bit) whether the device support ZPODD or ZPREADY and it will all just work(tm). The industry expectation is that ZPODD is just a transition state between current power management and ZPREADY.

Quoted 49 lines · James Bottomley

Quoted 42 lines · Tejun Heo

Hey, Aaron.

Quoted 6 lines · Aaron Lu

Quoted 4 lines

What I'm confused about is what autopm does for devices w/o zpodd. What happens then? Is it gonna leave power on for the device and, say, go on to suspend the controller? But, how would that work for, say, future devices with async notification for media events?

Maybe we shouldn't allow autopm for such devices?

Yeah, maybe. It would be nice to be able to automatically power off disks and ports which aren't being used tho.

Quoted 24 lines

Quoted 5 lines

That said, I can't say the snooping is pretty. It's a rather nasty thing to do. So, libata now wants information from the event polling in block layer, but reaching for block_device from ata_devices is nasty too. Hmmm... but aren't you already doing that to block polling on a powered down device?

I was feeling brain damaged by this for some time :-)

Basically, only ATA layer is aware of the power off thing, and sr knows nothing about this(or it is not supposed to know this, at least, this is what SCSI people think) and once powered off, I do not want the poll to disturb the device, so I need to block the poll. I can't come up with another way to achieve this except this nasty way.

James suggests me to keep the poll, but emulate the command. The problem with this is, the autopm for resume will kick in on each poll, so I'll need to decide if power up the ODD for this time's resume is needed in port's runtime resume callback. This made things complex and it also put too much logic in the resume callback, which is not desired. And even if I keep the ODD in powered off state by emulating this poll command, its ancestor devices will still be resumed, and I may need to do some trick in their resume callback to avoid needless power/state transitions. This doesn't feel like an elegant way to solve this either.

Quoted 4 lines

So yes, I'm still using this _nasty_ way to make the ODD stay in powered off state as long as possible. But if there is other elegant ways to solve this, I would appreciate it and happily using it. Personally, I hope we can make sr aware of ZPODD, that would make the pain gone.

Quoted 5 lines

I really think we need a way for (auto)pm and event polling to talk to each other so that autopm can tell event poll to sod off while pm is in effect. Trying to solve this from inside libata doesn't seem right. The problem, again, seems to be figuring out which hardware device maps to which block device. Hmmm... Any good ideas?

Quoted 4 lines

I've asked the PM people several times about this, because it's a real problem for almost everything: PM needs some type of top to bottom stack view, which the layering isolation we have within storage really doesn't cope with well. No real suggestion has been forthcoming.

Actually, I think that the particular case in question is really special and the fact that there's the pollig loop that user space is involved in doesn't make things more stratightforward.

And PM really doesn't need to see things top to bottom, but the polling needs to know what happens in the PM land. We need to be able to tell it "from now on tell user space that there are no events here". The question

is where to put that information so that it's accessible to all parts of the stack involved.

Quoted 10 lines

The reason I think it should be emulated (in the acpi layer, not libata, but as long as it's not in SCSI, I'm not so fussed where it ends up) is because ZPODD is the software equivalent of ZPREADY, which will be done in hardware and will be effectively invisible to autopm in the same way that SCSI (and ATA) power management is mostly invisible. If we currently do ZPREADY emulation in the low layer (i.e. ZPODD has exact ZPREADY emulation), we won't care (except for flipping the sofware bit) whether the device support ZPODD or ZPREADY and it will all just work(tm). The industry expectation is that ZPODD is just a transition state between current power management and ZPREADY.

Well, if you poll a ZPREADY-capable drive, it will go off and on in cycles transparently, but still it won't save as much energy as it can. We'll need to do something about the polling in that case too, it seems.

Thanks,
Rafael

Quoted 10 lines · James Bottomley

Quoted 5 lines

I really think we need a way for (auto)pm and event polling to talk to each other so that autopm can tell event poll to sod off while pm is in effect. Trying to solve this from inside libata doesn't seem right. The problem, again, seems to be figuring out which hardware device maps to which block device. Hmmm... Any good ideas?

I've asked the PM people several times about this, because it's a real problem for almost everything: PM needs some type of top to bottom stack view, which the layering isolation we have within storage really doesn't cope with well. No real suggestion has been forthcoming.

Actually, I think that the particular case in question is really special and the fact that there's the pollig loop that user space is involved in doesn't make things more stratightforward.

And PM really doesn't need to see things top to bottom, but the polling needs to know what happens in the PM land. We need to be able to tell it "from now on tell user space that there are no events here". The question is where to put that information so that it's accessible to all parts of the stack involved.

Quoted 10 lines

The reason I think it should be emulated (in the acpi layer, not libata, but as long as it's not in SCSI, I'm not so fussed where it ends up) is because ZPODD is the software equivalent of ZPREADY, which will be done in hardware and will be effectively invisible to autopm in the same way that SCSI (and ATA) power management is mostly invisible. If we currently do ZPREADY emulation in the low layer (i.e. ZPODD has exact ZPREADY emulation), we won't care (except for flipping the sofware bit) whether the device support ZPODD or ZPREADY and it will all just work(tm). The industry expectation is that ZPODD is just a transition state between current power management and ZPREADY.

Well, if you poll a ZPREADY-capable drive, it will go off and on in cycles transparently, but still it won't save as much energy as it can. We'll need to do something about the polling in that case too, it seems.

Quoted 16 lines · Rafael J. Wysocki

Quoted 10 lines · James Bottomley

Quoted 5 lines

I really think we need a way for (auto)pm and event polling to talk to each other so that autopm can tell event poll to sod off while pm is in effect. Trying to solve this from inside libata doesn't seem right. The problem, again, seems to be figuring out which hardware device maps to which block device. Hmmm... Any good ideas?

I've asked the PM people several times about this, because it's a real problem for almost everything: PM needs some type of top to bottom stack view, which the layering isolation we have within storage really doesn't cope with well. No real suggestion has been forthcoming.

Actually, I think that the particular case in question is really special and the fact that there's the pollig loop that user space is involved in doesn't make things more stratightforward.

Quoted 5 lines

And PM really doesn't need to see things top to bottom, but the polling needs to know what happens in the PM land. We need to be able to tell it "from now on tell user space that there are no events here". The question is where to put that information so that it's accessible to all parts of the stack involved.

Right, open to suggestions ...

Quoted 11 lines

Quoted 10 lines

The reason I think it should be emulated (in the acpi layer, not libata, but as long as it's not in SCSI, I'm not so fussed where it ends up) is because ZPODD is the software equivalent of ZPREADY, which will be done in hardware and will be effectively invisible to autopm in the same way that SCSI (and ATA) power management is mostly invisible. If we currently do ZPREADY emulation in the low layer (i.e. ZPODD has exact ZPREADY emulation), we won't care (except for flipping the sofware bit) whether the device support ZPODD or ZPREADY and it will all just work(tm). The industry expectation is that ZPODD is just a transition state between current power management and ZPREADY.

Quoted 3 lines

Well, if you poll a ZPREADY-capable drive, it will go off and on in cycles transparently, but still it won't save as much energy as it can. We'll need to do something about the polling in that case too, it seems.

No: with ZPREADY, the device effectively lies to the poll. The Spec says that when it powers off the mechanical pieces, it must reply from firmware to a certain preset emulations of SCSI commands and not wake from power off. These commands include TEST UNIT READY and a few others, so the device will happily reply to polls while being off (it replies with the original state before power was lost). When you issue actual medium access commands, or manually insert or remove media it will wake up.

That's why I think ZPODD should emulate this behaviour.

James

Quoted 49 lines · James Bottomley

Quoted 16 lines · Rafael J. Wysocki

Quoted 10 lines · James Bottomley

Quoted 5 lines

I really think we need a way for (auto)pm and event polling to talk to each other so that autopm can tell event poll to sod off while pm is in effect. Trying to solve this from inside libata doesn't seem right. The problem, again, seems to be figuring out which hardware device maps to which block device. Hmmm... Any good ideas?

I've asked the PM people several times about this, because it's a real problem for almost everything: PM needs some type of top to bottom stack view, which the layering isolation we have within storage really doesn't cope with well. No real suggestion has been forthcoming.

Actually, I think that the particular case in question is really special and the fact that there's the pollig loop that user space is involved in doesn't make things more stratightforward.

Quoted 5 lines

And PM really doesn't need to see things top to bottom, but the polling needs to know what happens in the PM land. We need to be able to tell it "from now on tell user space that there are no events here". The question is where to put that information so that it's accessible to all parts of the stack involved.

Right, open to suggestions ...

Quoted 11 lines

Quoted 10 lines

The reason I think it should be emulated (in the acpi layer, not libata, but as long as it's not in SCSI, I'm not so fussed where it ends up) is because ZPODD is the software equivalent of ZPREADY, which will be done in hardware and will be effectively invisible to autopm in the same way that SCSI (and ATA) power management is mostly invisible. If we currently do ZPREADY emulation in the low layer (i.e. ZPODD has exact ZPREADY emulation), we won't care (except for flipping the sofware bit) whether the device support ZPODD or ZPREADY and it will all just work(tm). The industry expectation is that ZPODD is just a transition state between current power management and ZPREADY.

Quoted 3 lines

Well, if you poll a ZPREADY-capable drive, it will go off and on in cycles transparently, but still it won't save as much energy as it can. We'll need to do something about the polling in that case too, it seems.

No: with ZPREADY, the device effectively lies to the poll. The Spec says that when it powers off the mechanical pieces, it must reply from firmware to a certain preset emulations of SCSI commands and not wake from power off. These commands include TEST UNIT READY and a few others, so the device will happily reply to polls while being off (it replies with the original state before power was lost). When you issue actual medium access commands, or manually insert or remove media it will wake up.

Quoted 1 line

That's why I think ZPODD should emulate this behaviour.

I suppose you are refering to section 15.3.5?

I don't think the SPEC says what the host software _must_ do, it's informative. And I agree that when possible, we should emulate the command without powering up the ODD, but if we can eliminate the noise, wouldn't that be better?

Quoted 50 lines · Aaron Lu

Quoted 49 lines · James Bottomley

Quoted 16 lines · Rafael J. Wysocki

Quoted 10 lines · James Bottomley

Quoted 5 lines

I really think we need a way for (auto)pm and event polling to talk to each other so that autopm can tell event poll to sod off while pm is in effect. Trying to solve this from inside libata doesn't seem right. The problem, again, seems to be figuring out which hardware device maps to which block device. Hmmm... Any good ideas?

I've asked the PM people several times about this, because it's a real problem for almost everything: PM needs some type of top to bottom stack view, which the layering isolation we have within storage really doesn't cope with well. No real suggestion has been forthcoming.

Actually, I think that the particular case in question is really special and the fact that there's the pollig loop that user space is involved in doesn't make things more stratightforward.

Quoted 5 lines

And PM really doesn't need to see things top to bottom, but the polling needs to know what happens in the PM land. We need to be able to tell it "from now on tell user space that there are no events here". The question is where to put that information so that it's accessible to all parts of the stack involved.

Right, open to suggestions ...

Quoted 11 lines

Quoted 10 lines

The reason I think it should be emulated (in the acpi layer, not libata, but as long as it's not in SCSI, I'm not so fussed where it ends up) is because ZPODD is the software equivalent of ZPREADY, which will be done in hardware and will be effectively invisible to autopm in the same way that SCSI (and ATA) power management is mostly invisible. If we currently do ZPREADY emulation in the low layer (i.e. ZPODD has exact ZPREADY emulation), we won't care (except for flipping the sofware bit) whether the device support ZPODD or ZPREADY and it will all just work(tm). The industry expectation is that ZPODD is just a transition state between current power management and ZPREADY.

Quoted 3 lines

Well, if you poll a ZPREADY-capable drive, it will go off and on in cycles transparently, but still it won't save as much energy as it can. We'll need to do something about the polling in that case too, it seems.

No: with ZPREADY, the device effectively lies to the poll. The Spec says that when it powers off the mechanical pieces, it must reply from firmware to a certain preset emulations of SCSI commands and not wake from power off. These commands include TEST UNIT READY and a few others, so the device will happily reply to polls while being off (it replies with the original state before power was lost). When you issue actual medium access commands, or manually insert or remove media it will wake up.

Quoted 3 lines

Quoted 1 line

That's why I think ZPODD should emulate this behaviour.

I suppose you are refering to section 15.3.5?

That's the recommendation for emulating ZPREADY in ZPODD devices, yes.

Quoted 4 lines

I don't think the SPEC says what the host software _must_ do, it's informative. And I agree that when possible, we should emulate the command without powering up the ODD, but if we can eliminate the noise, wouldn't that be better?

The way I look at it is we currently have no real power management for actual SCSI devices, we rely on the internal timers of the device to effect power management for us. ZPREADY fits right into this scheme (as I think it was designed to) so it seems odd to me that we would introduce a software PM based scheme for ZPODD, which is an interim spec until everything supports ZPREADY, and then go back to doing nothing for ZPREADY.

I'm amenable to a proposal that we *shouldn't* be doing nothing for SCSI PM and perhaps it should be plumbed into software PM, but it looks odd to me to have sofware PM for ZPODD but not for at least ZPREADY and probably for SCSI PM as well.

If we elect to do nothing about ZPREADY and SCSI PM, then I think ZPODD should be implemented in a way that emulates ZPREADY (i.e. as section 15.3.5 recommends).

James

Well, ZPREADY is not a power state that we can program the ODD to enter(figure 234 and table 323 of the SPEC), it servers more like an information provided by ODD to host so that host does not need to do TUR and then examine the sense code to decide if zero power ready status is satisfied but simply query ODD if its current power state is ZPREADY. So it's not that we program the device to go into ZPREADY power state and the ODD's power will be omitted.

The benefit of a ZPREADY capable ODD is that, when we need to decide if the ODD is in a zero power ready status, we can simply query the ODD by issuing a GET_EVENT_STATUS_NOTIFICATION and check the returned power class events, if it is in ZPREADY power state, then we can omit the power. To support ZPREADY, we just need some change to the zpready funtion, which currently uses sense code to check ZP ready status.

So this is my understanding of ZPREADY, and I don't see it as a total different thing with ZPODD, it just changes the way how host senses the zero power ready status. But if I was wrong, please kindly let me know, thanks.

I'm also curious about driving sleep from autopm, since mode page timers don't control the sleep transition.

Quoted 2 lines · James Bottomley

I'm also curious about driving sleep from autopm, since mode page timers don't control the sleep transition.

Is it feasible to do this the other way around? That is, to drive runtime suspend by noticing when the device decides to put itself into a low-power state?

Quoted 15 lines · Aaron Lu

Well, ZPREADY is not a power state that we can program the ODD to enter(figure 234 and table 323 of the SPEC), it servers more like an information provided by ODD to host so that host does not need to do TUR and then examine the sense code to decide if zero power ready status is satisfied but simply query ODD if its current power state is ZPREADY. So it's not that we program the device to go into ZPREADY power state and the ODD's power will be omitted.

The benefit of a ZPREADY capable ODD is that, when we need to decide if the ODD is in a zero power ready status, we can simply query the ODD by issuing a GET_EVENT_STATUS_NOTIFICATION and check the returned power class events, if it is in ZPREADY power state, then we can omit the power. To support ZPREADY, we just need some change to the zpready funtion, which currently uses sense code to check ZP ready status.

Quoted 4 lines

So this is my understanding of ZPREADY, and I don't see it as a total different thing with ZPODD, it just changes the way how host senses the zero power ready status. But if I was wrong, please kindly let me know, thanks.

My understanding is that a ZPREADY device may be capable of internal power down, meaning it doesn't necessarily need the host to omit the power. It depends what the difference is between Sleep and Off is (they're deliberately left as implementation defined in the standard, Ch 16, but the conditions of sleep are pretty onerous, so it sounds like most of the mechanics are powered down).

However, if you want to work it similarly to ZPODD, then the timeouts automatically transitions to ZPREADY, the device issues an event, we trap the event at the low level and omit power.

I'm also curious about driving sleep from autopm, since mode page timers don't control the sleep transition.

Quoted 16 lines · Rafael J. Wysocki

Quoted 10 lines · James Bottomley

Quoted 5 lines

I really think we need a way for (auto)pm and event polling to talk to each other so that autopm can tell event poll to sod off while pm is in effect. Trying to solve this from inside libata doesn't seem right. The problem, again, seems to be figuring out which hardware device maps to which block device. Hmmm... Any good ideas?

I've asked the PM people several times about this, because it's a real problem for almost everything: PM needs some type of top to bottom stack view, which the layering isolation we have within storage really doesn't cope with well. No real suggestion has been forthcoming.

Actually, I think that the particular case in question is really special and the fact that there's the pollig loop that user space is involved in doesn't make things more stratightforward.

Quoted 5 lines

And PM really doesn't need to see things top to bottom, but the polling needs to know what happens in the PM land. We need to be able to tell it "from now on tell user space that there are no events here". The question is where to put that information so that it's accessible to all parts of the stack involved.

Right, open to suggestions ...

Having considered that a bit more I'm now thinking that in fact the power state the device is in at the moment doesn't really matter, so the polling code need not really know what PM is doing. What it needs to know is that the device will generate a hardware event when something interesting happens, so it is not necessary to poll it.

In this particular case it is related to power management (apparently, hardware events will only be generated after the device has been put into ACPI D3cold, or so Aaron seems to be claiming), but it need not be in general, at least in principle.

It looks like we need an "event driven" flag that the can be set in the lower layers and read by the upper layers. I suppose this means it would need to be in struct device, but not necessarily in the PM-specific part of it.

Hey, Rafael.

Quoted 11 lines · Rafael J. Wysocki

Having considered that a bit more I'm now thinking that in fact the power state the device is in at the moment doesn't really matter, so the polling code need not really know what PM is doing. What it needs to know is that the device will generate a hardware event when something interesting happens, so it is not necessary to poll it.

In this particular case it is related to power management (apparently, hardware events will only be generated after the device has been put into ACPI D3cold, or so Aaron seems to be claiming), but it need not be in general, at least in principle.

Quoted 3 lines

It looks like we need an "event driven" flag that the can be set in the lower layers and read by the upper layers. I suppose this means it would need to be in struct device, but not necessarily in the PM-specific part of it.

We already have that. That's what gendisk->async_events is for (as opposed to gendisk->events). If all events are async_events, block won't poll for events, but I'm not sure that's the golden bullet.

* None implements async_events yet and an API is missing -

disk_check_events() - which is trivial to add, but it's the same story. We'll need a mechanism to shoot up notification from libata to block layer. It's admittedly easier to justify routing through SCSI tho. So, we're mostly shifting the problem. Given that async events is nice to have, so it isn't a bad idea.

* Still dunno much about zpodd but IIUC the notification from zero-power is via ACPI. To advertise that the device doesn't need

polling, it should also be able to do async notification while powered up, which isn't covered by zpodd but ATA async notification.

So, ummm... that's another obstacle. If zpodd requires the device to support ATA async notification, it might not be too bad tho.

Hey, Rafael.

Quoted 6 lines · Rafael J. Wysocki

Having considered that a bit more I'm now thinking that in fact the power state the device is in at the moment doesn't really matter, so the polling code need not really know what PM is doing. What it needs to know is that the device will generate a hardware event when something interesting happens, so it is not necessary to poll it.

Quoted 8 lines

In this particular case it is related to power management (apparently, hardware events will only be generated after the device has been put into ACPI D3cold, or so Aaron seems to be claiming), but it need not be in general, at least in principle.

It looks like we need an "event driven" flag that the can be set in the lower layers and read by the upper layers. I suppose this means it would need to be in struct device, but not necessarily in the PM-specific part of it.

We already have that. That's what gendisk->async_events is for (as opposed to gendisk->events). If all events are async_events, block won't poll for events, but I'm not sure that's the golden bullet.

* None implements async_events yet and an API is missing - disk_check_events() - which is trivial to add, but it's the same story. We'll need a mechanism to shoot up notification from libata to block layer. It's admittedly easier to justify routing through SCSI tho. So, we're mostly shifting the problem. Given that async events is nice to have, so it isn't a bad idea.

* Still dunno much about zpodd but IIUC the notification from zero-power is via ACPI. To advertise that the device doesn't need polling, it should also be able to do async notification while powered up, which isn't covered by zpodd but ATA async notification. So, ummm... that's another obstacle. If zpodd requires the device to support ATA async notification, it might not be too bad tho.

Quoted 22 lines · Tejun Heo

Hey, Rafael.

Quoted 6 lines · Rafael J. Wysocki

Having considered that a bit more I'm now thinking that in fact the power state the device is in at the moment doesn't really matter, so the polling code need not really know what PM is doing. What it needs to know is that the device will generate a hardware event when something interesting happens, so it is not necessary to poll it.

Quoted 8 lines

In this particular case it is related to power management (apparently, hardware events will only be generated after the device has been put into ACPI D3cold, or so Aaron seems to be claiming), but it need not be in general, at least in principle.

It looks like we need an "event driven" flag that the can be set in the lower layers and read by the upper layers. I suppose this means it would need to be in struct device, but not necessarily in the PM-specific part of it.

We already have that. That's what gendisk->async_events is for (as opposed to gendisk->events). If all events are async_events, block won't poll for events, but I'm not sure that's the golden bullet.

Quoted 6 lines

* None implements async_events yet and an API is missing - disk_check_events() - which is trivial to add, but it's the same story. We'll need a mechanism to shoot up notification from libata to block layer. It's admittedly easier to justify routing through SCSI tho. So, we're mostly shifting the problem. Given that async events is nice to have, so it isn't a bad idea.

Could we drive it in the polling code? As in, if we set a flag to say we're event driven and please don't bother us, we could just respond to the poll with the last known state (this would probably have to be in SCSI somewhere since most polls are Test Unit Readys). That way ZPODD sets this flag when the device powers down and unsets it when it powers up.

James

Quoted 6 lines

* Still dunno much about zpodd but IIUC the notification from zero-power is via ACPI. To advertise that the device doesn't need polling, it should also be able to do async notification while powered up, which isn't covered by zpodd but ATA async notification. So, ummm... that's another obstacle. If zpodd requires the device to support ATA async notification, it might not be too bad tho.

Quoted 30 lines · James Bottomley

Quoted 22 lines · Tejun Heo

Hey, Rafael.

Quoted 6 lines · Rafael J. Wysocki

Having considered that a bit more I'm now thinking that in fact the power state the device is in at the moment doesn't really matter, so the polling code need not really know what PM is doing. What it needs to know is that the device will generate a hardware event when something interesting happens, so it is not necessary to poll it.

Quoted 8 lines

In this particular case it is related to power management (apparently, hardware events will only be generated after the device has been put into ACPI D3cold, or so Aaron seems to be claiming), but it need not be in general, at least in principle.

It looks like we need an "event driven" flag that the can be set in the lower layers and read by the upper layers. I suppose this means it would need to be in struct device, but not necessarily in the PM-specific part of it.

We already have that. That's what gendisk->async_events is for (as opposed to gendisk->events). If all events are async_events, block won't poll for events, but I'm not sure that's the golden bullet.

Quoted 6 lines

* None implements async_events yet and an API is missing - disk_check_events() - which is trivial to add, but it's the same story. We'll need a mechanism to shoot up notification from libata to block layer. It's admittedly easier to justify routing through SCSI tho. So, we're mostly shifting the problem. Given that async events is nice to have, so it isn't a bad idea.

Quoted 6 lines

Could we drive it in the polling code? As in, if we set a flag to say we're event driven and please don't bother us, we could just respond to the poll with the last known state (this would probably have to be in SCSI somewhere since most polls are Test Unit Readys). That way ZPODD sets this flag when the device powers down and unsets it when it powers up.

Does it mean I can do something like this:

drivers/scsi/sr.c +4 -1

index 5fc97d2..219820c 100644
@@ -578,7 +578,10 @@ static unsigned int sr_block_check_events(struct gendisk *disk,
 					  unsigned int clearing)
 {
 	struct scsi_cd *cd = scsi_cd(disk);
-	return cdrom_check_events(&cd->cdi, clearing);
+	if (!cd->device->event_driven)
+		return cdrom_check_events(&cd->cdi, clearing);
+	else
+		return 0;
 }
 
 static int sr_block_revalidate_disk(struct gendisk *disk)

include/scsi/scsi_device.h +0 -0

index e65c62e..1756151 100644

Then when ZPODD is powered off, it will set this flag; and unset it when it is powered up.

Quoted 61 lines · Aaron Lu

Quoted 30 lines · James Bottomley

Quoted 22 lines · Tejun Heo

Hey, Rafael.

Quoted 6 lines · Rafael J. Wysocki

Having considered that a bit more I'm now thinking that in fact the power state the device is in at the moment doesn't really matter, so the polling code need not really know what PM is doing. What it needs to know is that the device will generate a hardware event when something interesting happens, so it is not necessary to poll it.

Quoted 8 lines

In this particular case it is related to power management (apparently, hardware events will only be generated after the device has been put into ACPI D3cold, or so Aaron seems to be claiming), but it need not be in general, at least in principle.

It looks like we need an "event driven" flag that the can be set in the lower layers and read by the upper layers. I suppose this means it would need to be in struct device, but not necessarily in the PM-specific part of it.

We already have that. That's what gendisk->async_events is for (as opposed to gendisk->events). If all events are async_events, block won't poll for events, but I'm not sure that's the golden bullet.

Quoted 6 lines

* None implements async_events yet and an API is missing - disk_check_events() - which is trivial to add, but it's the same story. We'll need a mechanism to shoot up notification from libata to block layer. It's admittedly easier to justify routing through SCSI tho. So, we're mostly shifting the problem. Given that async events is nice to have, so it isn't a bad idea.

Quoted 6 lines

Could we drive it in the polling code? As in, if we set a flag to say we're event driven and please don't bother us, we could just respond to the poll with the last known state (this would probably have to be in SCSI somewhere since most polls are Test Unit Readys). That way ZPODD sets this flag when the device powers down and unsets it when it powers up.

Does it mean I can do something like this:

drivers/scsi/sr.c +4 -1

index 5fc97d2..219820c 100644
@@ -578,7 +578,10 @@ static unsigned int sr_block_check_events(struct gendisk *disk,
 					  unsigned int clearing)
 {
 	struct scsi_cd *cd = scsi_cd(disk);
-	return cdrom_check_events(&cd->cdi, clearing);
+	if (!cd->device->event_driven)
+		return cdrom_check_events(&cd->cdi, clearing);
+	else
+		return 0;
 }
 
 static int sr_block_revalidate_disk(struct gendisk *disk)

include/scsi/scsi_device.h +0 -0

index e65c62e..1756151 100644

include/scsi/scsi_device.h struct scsi_device { · line 166

Quoted 7 lines

include/scsi/scsi_device.h +1 -0

@@ -160,6 +160,7 @@ struct scsi_device {
 	unsigned can_power_off:1; /* Device supports runtime power off */
 	unsigned wce_default_on:1;	/* Cache is ON by default */
 	unsigned no_dif:1;	/* T10 PI (DIF) should be disabled */
+	unsigned event_driven:1; /* No need to poll the device */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */

Yes, but if we can get away with doing that, it should be in genhd because it's completely generic.

I was imagining we'd have to fake the reply to the test unit ready or some other commands, which is why it would need to be in sr.c

The check events code is Tejun's baby, if he's OK with it then just do it in genhd.c

Quoted 3 lines

Then when ZPODD is powered off, it will set this flag; and unset it when it is powered up.

Right.

James

include/scsi/scsi_device.h struct scsi_device { · line 166

Quoted 12 lines

include/scsi/scsi_device.h +1 -0

index e65c62e..1756151 100644
@@ -160,6 +160,7 @@ struct scsi_device {
 	unsigned can_power_off:1; /* Device supports runtime power off */
 	unsigned wce_default_on:1;	/* Cache is ON by default */
 	unsigned no_dif:1;	/* T10 PI (DIF) should be disabled */
+	unsigned event_driven:1; /* No need to poll the device */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */

Yes, but if we can get away with doing that, it should be in genhd because it's completely generic.

I was imagining we'd have to fake the reply to the test unit ready or some other commands, which is why it would need to be in sr.c

The check events code is Tejun's baby, if he's OK with it then just do it in genhd.c

Hello, James.

Quoted 19 lines · James Bottomley

include/scsi/scsi_device.h struct scsi_device { · line 166

Quoted 12 lines

include/scsi/scsi_device.h +1 -0

index e65c62e..1756151 100644
@@ -160,6 +160,7 @@ struct scsi_device {
  	unsigned can_power_off:1; /* Device supports runtime power off */
  	unsigned wce_default_on:1;	/* Cache is ON by default */
  	unsigned no_dif:1;	/* T10 PI (DIF) should be disabled */
+	unsigned event_driven:1; /* No need to poll the device */
 
  	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
  	struct list_head event_list;	/* asserted events */

Yes, but if we can get away with doing that, it should be in genhd because it's completely generic.

I was imagining we'd have to fake the reply to the test unit ready or some other commands, which is why it would need to be in sr.c

Quoted 2 lines

The check events code is Tejun's baby, if he's OK with it then just do it in genhd.c

The problem here is there's no easy to reach genhd from libata (or the other way around) without going through sr. I think we're gonna have to have something in sr one way or the other.

Quoted 25 lines · Tejun Heo

Hello, James.

Quoted 21 lines · James Bottomley

include/scsi/scsi_device.h struct scsi_device { · line 166

Quoted 12 lines

include/scsi/scsi_device.h +1 -0

index e65c62e..1756151 100644
@@ -160,6 +160,7 @@ struct scsi_device {
  	unsigned can_power_off:1; /* Device supports runtime power off */
  	unsigned wce_default_on:1;	/* Cache is ON by default */
  	unsigned no_dif:1;	/* T10 PI (DIF) should be disabled */
+	unsigned event_driven:1; /* No need to poll the device */
 
  	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
  	struct list_head event_list;	/* asserted events */

Yes, but if we can get away with doing that, it should be in genhd because it's completely generic.

I was imagining we'd have to fake the reply to the test unit ready or some other commands, which is why it would need to be in sr.c

The check events code is Tejun's baby, if he's OK with it then just do it in genhd.c

Quoted 3 lines

The problem here is there's no easy to reach genhd from libata (or the other way around) without going through sr. I think we're gonna have to have something in sr one way or the other.

...which is precisely as I said when v1 of this ZPODD patchset appeared.

sr modifications cannot be avoided.

Jeff

Hello, James.

Quoted 19 lines · James Bottomley

include/scsi/scsi_device.h struct scsi_device { · line 166

Quoted 12 lines

include/scsi/scsi_device.h +1 -0

index e65c62e..1756151 100644
@@ -160,6 +160,7 @@ struct scsi_device {
 	unsigned can_power_off:1; /* Device supports runtime power off */
 	unsigned wce_default_on:1;	/* Cache is ON by default */
 	unsigned no_dif:1;	/* T10 PI (DIF) should be disabled */
+	unsigned event_driven:1; /* No need to poll the device */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */

Yes, but if we can get away with doing that, it should be in genhd because it's completely generic.

I was imagining we'd have to fake the reply to the test unit ready or some other commands, which is why it would need to be in sr.c

Quoted 2 lines

The check events code is Tejun's baby, if he's OK with it then just do it in genhd.c

The problem here is there's no easy to reach genhd from libata (or the other way around) without going through sr. I think we're gonna have to have something in sr one way or the other.

Quoted 22 lines · Tejun Heo

Hello, James.

Quoted 19 lines · James Bottomley

include/scsi/scsi_device.h struct scsi_device { · line 166

Quoted 12 lines

include/scsi/scsi_device.h +1 -0

index e65c62e..1756151 100644
@@ -160,6 +160,7 @@ struct scsi_device {
 	unsigned can_power_off:1; /* Device supports runtime power off */
 	unsigned wce_default_on:1;	/* Cache is ON by default */
 	unsigned no_dif:1;	/* T10 PI (DIF) should be disabled */
+	unsigned event_driven:1; /* No need to poll the device */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */

Yes, but if we can get away with doing that, it should be in genhd because it's completely generic.

I was imagining we'd have to fake the reply to the test unit ready or some other commands, which is why it would need to be in sr.c

Quoted 6 lines

Quoted 2 lines

The check events code is Tejun's baby, if he's OK with it then just do it in genhd.c

The problem here is there's no easy to reach genhd from libata (or the other way around) without going through sr. I think we're gonna have to have something in sr one way or the other.

Can't we do that via an event? It's a bit clunky because we need the callback in the layer that sees the sdev, which is libata-scsi, we just need an analogue of ata_scsi_media_change_notify, but ignoring and allowing polling is essentially event driven as well, so it should all work. We'll need a listener in genhd, which might be trickier.

This may also work as the more generic route for stuff where we can't connect the bottom to the top of the stack (which looks like a problem we'll begin wrestling with a lot now).

James

Quoted 30 lines · James Bottomley

Quoted 22 lines · Tejun Heo

Hello, James.

Quoted 19 lines · James Bottomley

include/scsi/scsi_device.h struct scsi_device { · line 166

Quoted 12 lines

include/scsi/scsi_device.h +1 -0

index e65c62e..1756151 100644
@@ -160,6 +160,7 @@ struct scsi_device {
 	unsigned can_power_off:1; /* Device supports runtime power off */
 	unsigned wce_default_on:1;	/* Cache is ON by default */
 	unsigned no_dif:1;	/* T10 PI (DIF) should be disabled */
+	unsigned event_driven:1; /* No need to poll the device */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */

Yes, but if we can get away with doing that, it should be in genhd because it's completely generic.

I was imagining we'd have to fake the reply to the test unit ready or some other commands, which is why it would need to be in sr.c

Quoted 6 lines

Quoted 2 lines

The check events code is Tejun's baby, if he's OK with it then just do it in genhd.c

The problem here is there's no easy to reach genhd from libata (or the other way around) without going through sr. I think we're gonna have to have something in sr one way or the other.

Quoted 5 lines

Can't we do that via an event? It's a bit clunky because we need the callback in the layer that sees the sdev, which is libata-scsi, we just need an analogue of ata_scsi_media_change_notify, but ignoring and allowing polling is essentially event driven as well, so it should all work. We'll need a listener in genhd, which might be trickier.

Hi Tejun,

Do you have any comments on James' suggestion? I want to know your opinion, thanks.

Hi James,

Do you mean we start a thread in genhd that listen to events, so that some driver can send an event to genhd about if the device should be polled? I'm thinking where to store such information. If store it in struct disk_events, then we will need to know which gendisk is for the device, but how? Maybe by loop scan all gendisk's driverfs_dev? If store it in struct device, then we do not need to send the event but just set a flag in sturct device in libata, and let genhd check this flag when poll. So I don't quite understand this, can you please elaborate? Thanks.

Thanks,
Aaron

Quoted 8 lines

This may also work as the more generic route for stuff where we can't connect the bottom to the top of the stack (which looks like a problem we'll begin wrestling with a lot now).

James

Quoted 9 lines

Quoted 9 lines

Quoted 3 lines

The problem here is there's no easy to reach genhd from libata (or the other way around) without going through sr. I think we're gonna have to have something in sr one way or the other.

Can't we do that via an event? It's a bit clunky because we need the callback in the layer that sees the sdev, which is libata-scsi, we just need an analogue of ata_scsi_media_change_notify, but ignoring and allowing polling is essentially event driven as well, so it should all work. We'll need a listener in genhd, which might be trickier.

A colleague of mine reminded me that it's impolite to write something like this, and here is my apology. I didn't mean to be rude, I'm sorry for this if it made you feel uncomfortable.

Hello, guys.

Quoted 9 lines · Aaron Lu

Quoted 9 lines

Quoted 9 lines

Quoted 3 lines

The problem here is there's no easy to reach genhd from libata (or the other way around) without going through sr. I think we're gonna have to have something in sr one way or the other.

Can't we do that via an event? It's a bit clunky because we need the callback in the layer that sees the sdev, which is libata-scsi, we just need an analogue of ata_scsi_media_change_notify, but ignoring and allowing polling is essentially event driven as well, so it should all work. We'll need a listener in genhd, which might be trickier.

I'm not really following what you mean. Can you please elaborate? One way or the other, doesn't the notification have to bubble up through SCSI?

Quoted 3 lines

A colleague of mine reminded me that it's impolite to write something like this, and here is my apology. I didn't mean to be rude, I'm sorry for this if it made you feel uncomfortable.

Oh, no, it's not rude at all. I was just being distracted w/ other stuff and lazy. Sorry about that.

Thanks.

Hello, James.

Quoted 19 lines · James Bottomley

include/scsi/scsi_device.h struct scsi_device { · line 166

Quoted 12 lines

include/scsi/scsi_device.h +1 -0

index e65c62e..1756151 100644
@@ -160,6 +160,7 @@ struct scsi_device {
 	unsigned can_power_off:1; /* Device supports runtime power off */
 	unsigned wce_default_on:1;	/* Cache is ON by default */
 	unsigned no_dif:1;	/* T10 PI (DIF) should be disabled */
+	unsigned event_driven:1; /* No need to poll the device */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */

Yes, but if we can get away with doing that, it should be in genhd because it's completely generic.

I was imagining we'd have to fake the reply to the test unit ready or some other commands, which is why it would need to be in sr.c

Quoted 2 lines

The check events code is Tejun's baby, if he's OK with it then just do it in genhd.c

The problem here is there's no easy to reach genhd from libata (or the other way around) without going through sr. I think we're gonna have to have something in sr one way or the other.

block/genhd.c +21 -0

+static int is_gendisk_part0(struct device *dev, void *data)
+{
+	struct device **child = data;
+
+	if (dev->class == &block_class && dev->type == &disk_type) {
+		*child = dev;
+		return 1;
+	} else
+		return 0;
+}
+
+/**
+ * disk_from_device - Get the gendisk pointer for this device.
+ * @dev: the device this gendisk is created for, i.e. gendisk->driverfs_dev
+ *
+ * LLD sometimes need to play with the gendisk without HLD's aware,
+ * this routine gives LLD the required access to gendisk.
+ *
+ * CONTEXT:
+ * Don't care.
+ */

Then together with disk_try_block_events and disk_unblock_events, we can avoid touching SCSI layer to let ODD stay in zero power state.

Hello, Aaron.

Quoted 27 lines · Aaron Lu

block/genhd.c +27 -0

+static int is_gendisk_part0(struct device *dev, void *data)
+{
+	struct device **child = data;
+
+	if (dev->class == &block_class && dev->type == &disk_type) {
+		*child = dev;
+		return 1;
+	} else
+		return 0;
+}
+
+/**
+ * disk_from_device - Get the gendisk pointer for this device.
+ * @dev: the device this gendisk is created for, i.e. gendisk->driverfs_dev
+ *
+ * LLD sometimes need to play with the gendisk without HLD's aware,
+ * this routine gives LLD the required access to gendisk.
+ *
+ * CONTEXT:
+ * Don't care.
+ */
+struct gendisk *disk_from_device(struct device *dev)
+{
+	struct device *child;
+
+	if (device_for_each_child(dev, &child, is_gendisk_part0))
+		return dev_to_disk(child);

block/genhd.c static void disk_events_workfn(struct work_struct *work) · line 1652

Quoted 4 lines

block/genhd.c +4 -0

+	else
+		return NULL;
+}
+EXPORT_SYMBOL(disk_from_device);

This is really a round-about way to find out the matching device and it wouldn't work if the disk device nests deeper. Doesn't really look like a good idea to me.

Quoted 3 lines

Then together with disk_try_block_events and disk_unblock_events, we can avoid touching SCSI layer to let ODD stay in zero power state.

Also, I'd much prefer something along the line of
block_events_nowait() instead of try_block.

Quoted 3 lines

This is really a round-about way to find out the matching device and it wouldn't work if the disk device nests deeper. Doesn't really look like a good idea to me.

I don't quite understand the 'disk device nests deeper' case, can you please elaborate? My understanding is, as long as the disk's part0 device has a parent, this function should work. For LLDs want to take

Hello,

Quoted 7 lines · Aaron Lu

Quoted 3 lines

This is really a round-about way to find out the matching device and it wouldn't work if the disk device nests deeper. Doesn't really look like a good idea to me.

I don't quite understand the 'disk device nests deeper' case, can you please elaborate? My understanding is, as long as the disk's part0 device has a parent, this function should work. For LLDs want to take

Hmmm, maybe I misread but it looked like it wouldn't work if there are intermediate nodes between the parent device and part0. It might not happen for sata but I don't think it's a good idea to assume that the part0 and hardware device are connected directly.

In general, it's a quite roundabout way to do it. Let's just push it through SCSI. That's how everything else is routed after all. It's confusing to do this one differently.

Hey, Rafael.

Quoted 6 lines · Rafael J. Wysocki

Having considered that a bit more I'm now thinking that in fact the power state the device is in at the moment doesn't really matter, so the polling code need not really know what PM is doing. What it needs to know is that the device will generate a hardware event when something interesting happens, so it is not necessary to poll it.

Quoted 8 lines

In this particular case it is related to power management (apparently, hardware events will only be generated after the device has been put into ACPI D3cold, or so Aaron seems to be claiming), but it need not be in general, at least in principle.

It looks like we need an "event driven" flag that the can be set in the lower layers and read by the upper layers. I suppose this means it would need to be in struct device, but not necessarily in the PM-specific part of it.

We already have that. That's what gendisk->async_events is for (as opposed to gendisk->events). If all events are async_events, block won't poll for events, but I'm not sure that's the golden bullet.

* None implements async_events yet and an API is missing - disk_check_events() - which is trivial to add, but it's the same story. We'll need a mechanism to shoot up notification from libata to block layer. It's admittedly easier to justify routing through

SCSI tho. So, we're mostly shifting the problem. Given that async events is nice to have, so it isn't a bad idea.

* Still dunno much about zpodd but IIUC the notification from zero-power is via ACPI. To advertise that the device doesn't need polling, it should also be able to do async notification while powered up, which isn't covered by zpodd but ATA async notification. So, ummm... that's another obstacle. If zpodd requires the device to support ATA async notification, it might not be too bad tho.

Thanks.

Quoted 18 lines · Tejun Heo

Hey, Rafael.

Quoted 6 lines · Rafael J. Wysocki

Having considered that a bit more I'm now thinking that in fact the power state the device is in at the moment doesn't really matter, so the polling code need not really know what PM is doing. What it needs to know is that the device will generate a hardware event when something interesting happens, so it is not necessary to poll it.

Quoted 8 lines

In this particular case it is related to power management (apparently, hardware events will only be generated after the device has been put into ACPI D3cold, or so Aaron seems to be claiming), but it need not be in general, at least in principle.

It looks like we need an "event driven" flag that the can be set in the lower layers and read by the upper layers. I suppose this means it would need to be in struct device, but not necessarily in the PM-specific part of it.

Quoted 8 lines

We already have that. That's what gendisk->async_events is for (as opposed to gendisk->events). If all events are async_events, block won't poll for events, but I'm not sure that's the golden bullet.

* None implements async_events yet and an API is missing - disk_check_events() - which is trivial to add, but it's the same story. We'll need a mechanism to shoot up notification from libata to block layer. It's admittedly easier to justify routing through

I don't see a way to do this, since libata has no chance of accessing the gendisk pointer. Or we can add a new field to struct device, something like no_poll, but I don't think it is the right thing to do, as not all devices are block ones.

Any other suggestions/ideas please?

Hey, Aaron.

Quoted 5 lines · Aaron Lu

Quoted 4 lines

What I'm confused about is what autopm does for devices w/o zpodd. What happens then? Is it gonna leave power on for the device and, say, go on to suspend the controller? But, how would that work for, say, future devices with async notification for media events?

Quoted 1 line

Maybe we shouldn't allow autopm for such devices?

Yeah, maybe. It would be nice to be able to automatically power off disks and ports which aren't being used tho.

Quoted 25 lines

Quoted 6 lines

That said, I can't say the snooping is pretty. It's a rather nasty thing to do. So, libata now wants information from the event polling in block layer, but reaching for block_device from ata_devices is nasty too. Hmmm... but aren't you already doing that to block polling on a powered down device?

I was feeling brain damaged by this for some time :-)

Basically, only ATA layer is aware of the power off thing, and sr knows nothing about this(or it is not supposed to know this, at least, this is what SCSI people think) and once powered off, I do not want the poll to disturb the device, so I need to block the poll. I can't come up with another way to achieve this except this nasty way.

James suggests me to keep the poll, but emulate the command. The problem with this is, the autopm for resume will kick in on each poll, so I'll need to decide if power up the ODD for this time's resume is needed in port's runtime resume callback. This made things complex and it also put too much logic in the resume callback, which is not desired. And even if I keep the ODD in powered off state by emulating this poll command, its ancestor devices will still be resumed, and I may need to do some trick in their resume callback to avoid needless power/state transitions. This doesn't feel like an elegant way to solve this either.

Quoted 4 lines

So yes, I'm still using this _nasty_ way to make the ODD stay in powered off state as long as possible. But if there is other elegant ways to solve this, I would appreciate it and happily using it. Personally, I hope we can make sr aware of ZPODD, that would make the pain gone.

I really think we need a way for (auto)pm and event polling to talk to each other so that autopm can tell event poll to sod off while pm is in effect. Trying to solve this from inside libata doesn't seem right. The problem, again, seems to be figuring out which hardware device maps to which block device. Hmmm... Any good ideas?

Quoted 5 lines · Tejun Heo

I really think we need a way for (auto)pm and event polling to talk to each other so that autopm can tell event poll to sod off while pm is in effect. Trying to solve this from inside libata doesn't seem right. The problem, again, seems to be figuring out which hardware device maps to which block device. Hmmm... Any good ideas?

A possible way of doing this is using pm qos.

We currently have 2 pm qos flags, NO_POWER_OFF and REMOTE_WAKEUP, and we can add another one: NO_POLL, use it like the following:
1 Set the NO_POLL pm qos flag when the underlying driver thinks it is no
longer necessary. In the ZPODD's case, it should be set when the device is to be powered off;
2 Clear it when poll is necessary again. In the ZPODD's case, when power
is re-gained, this flag will be cleared.

3 In the disk_events_workfn, check if this flag is set, if so, simply
return.

The disk->driverfs_dev can be used to host the pm qos flag, ATA layer can access it through ata_device->sdev->sdev_gendev.

Is this OK?

Thanks,
Aaron

Quoted 16 lines · Aaron Lu

Quoted 5 lines · Tejun Heo

I really think we need a way for (auto)pm and event polling to talk to each other so that autopm can tell event poll to sod off while pm is in effect. Trying to solve this from inside libata doesn't seem right. The problem, again, seems to be figuring out which hardware device maps to which block device. Hmmm... Any good ideas?

A possible way of doing this is using pm qos.

We currently have 2 pm qos flags, NO_POWER_OFF and REMOTE_WAKEUP, and we can add another one: NO_POLL, use it like the following:
1 Set the NO_POLL pm qos flag when the underlying driver thinks it is no
longer necessary. In the ZPODD's case, it should be set when the device is to be powered off;
2 Clear it when poll is necessary again. In the ZPODD's case, when power
is re-gained, this flag will be cleared.

Quoted 2 lines

3 In the disk_events_workfn, check if this flag is set, if so, simply
return.

It should be, skip calling disk->fops->check_events, but still queue the work for next time's poll.

-Aaron

Quoted 5 lines

The disk->driverfs_dev can be used to host the pm qos flag, ATA layer can access it through ata_device->sdev->sdev_gendev.

Is this OK?

Quoted 31 lines · Aaron Lu

Quoted 18 lines · Aaron Lu

Quoted 5 lines · Tejun Heo

I really think we need a way for (auto)pm and event polling to talk to each other so that autopm can tell event poll to sod off while pm is in effect. Trying to solve this from inside libata doesn't seem right. The problem, again, seems to be figuring out which hardware device maps to which block device. Hmmm... Any good ideas?

A possible way of doing this is using pm qos.

We currently have 2 pm qos flags, NO_POWER_OFF and REMOTE_WAKEUP, and we can add another one: NO_POLL, use it like the following:
1 Set the NO_POLL pm qos flag when the underlying driver thinks it is no
longer necessary. In the ZPODD's case, it should be set when the device is to be powered off;
2 Clear it when poll is necessary again. In the ZPODD's case, when power
is re-gained, this flag will be cleared.

Quoted 2 lines

3 In the disk_events_workfn, check if this flag is set, if so, simply
return.

It should be, skip calling disk->fops->check_events, but still queue the work for next time's poll.

-Aaron

Quoted 4 lines

The disk->driverfs_dev can be used to host the pm qos flag, ATA layer can access it through ata_device->sdev->sdev_gendev.

Quoted 1 line

Quoted 1 line

Is this OK?

No, I don't think so. PM QoS is about telling the layer that will put the device into low-power states what states are to be taken into consideration. In this case, however, we need to tell someone else that the device has been turned off. Clearly, we need a way to do that, but not through PM QoS.

Did you consider using pm_runtime_suspended() to check the device status?

Rafael

Quoted 39 lines · Rafael J. Wysocki

Quoted 19 lines · Aaron Lu

Quoted 16 lines · Aaron Lu

Quoted 5 lines · Tejun Heo

I really think we need a way for (auto)pm and event polling to talk to each other so that autopm can tell event poll to sod off while pm is in effect. Trying to solve this from inside libata doesn't seem right. The problem, again, seems to be figuring out which hardware device maps to which block device. Hmmm... Any good ideas?

A possible way of doing this is using pm qos.

We currently have 2 pm qos flags, NO_POWER_OFF and REMOTE_WAKEUP, and we can add another one: NO_POLL, use it like the following:
1 Set the NO_POLL pm qos flag when the underlying driver thinks it is no
longer necessary. In the ZPODD's case, it should be set when the device is to be powered off;
2 Clear it when poll is necessary again. In the ZPODD's case, when power
is re-gained, this flag will be cleared.

Quoted 13 lines

Quoted 2 lines

3 In the disk_events_workfn, check if this flag is set, if so, simply
return.

It should be, skip calling disk->fops->check_events, but still queue the work for next time's poll.

-Aaron

Quoted 5 lines

The disk->driverfs_dev can be used to host the pm qos flag, ATA layer can access it through ata_device->sdev->sdev_gendev.

Is this OK?

No, I don't think so. PM QoS is about telling the layer that will put the device into low-power states what states are to be taken into consideration. In this case, however, we need to tell someone else that the device has been turned off. Clearly, we need a way to do that, but not through PM QoS.

Quoted 1 line

Did you consider using pm_runtime_suspended() to check the device status?

The problem is, a device can be in runtime suspended state while still needs to be polled...

Quoted 40 lines · Aaron Lu

Quoted 39 lines · Rafael J. Wysocki

Quoted 19 lines · Aaron Lu

Quoted 16 lines · Aaron Lu

Quoted 5 lines · Tejun Heo

I really think we need a way for (auto)pm and event polling to talk to each other so that autopm can tell event poll to sod off while pm is in effect. Trying to solve this from inside libata doesn't seem right. The problem, again, seems to be figuring out which hardware device maps to which block device. Hmmm... Any good ideas?

A possible way of doing this is using pm qos.

We currently have 2 pm qos flags, NO_POWER_OFF and REMOTE_WAKEUP, and we can add another one: NO_POLL, use it like the following:
1 Set the NO_POLL pm qos flag when the underlying driver thinks it is no
longer necessary. In the ZPODD's case, it should be set when the device is to be powered off;
2 Clear it when poll is necessary again. In the ZPODD's case, when power
is re-gained, this flag will be cleared.

Quoted 13 lines

Quoted 2 lines

3 In the disk_events_workfn, check if this flag is set, if so, simply
return.

It should be, skip calling disk->fops->check_events, but still queue the work for next time's poll.

-Aaron

Quoted 5 lines

The disk->driverfs_dev can be used to host the pm qos flag, ATA layer can access it through ata_device->sdev->sdev_gendev.

Is this OK?

No, I don't think so. PM QoS is about telling the layer that will put the device into low-power states what states are to be taken into consideration. In this case, however, we need to tell someone else that the device has been turned off. Clearly, we need a way to do that, but not through PM QoS.

Quoted 4 lines

Quoted 1 line

Did you consider using pm_runtime_suspended() to check the device status?

The problem is, a device can be in runtime suspended state while still needs to be polled...

Well, maybe this is the problem, then? Why does it need to be polled when suspended?

Rafael

Quoted 41 lines · Rafael J. Wysocki

Quoted 40 lines · Aaron Lu

Quoted 39 lines · Rafael J. Wysocki

Quoted 19 lines · Aaron Lu

Quoted 16 lines · Aaron Lu

Quoted 5 lines · Tejun Heo

I really think we need a way for (auto)pm and event polling to talk to each other so that autopm can tell event poll to sod off while pm is in effect. Trying to solve this from inside libata doesn't seem right. The problem, again, seems to be figuring out which hardware device maps to which block device. Hmmm... Any good ideas?

A possible way of doing this is using pm qos.

We currently have 2 pm qos flags, NO_POWER_OFF and REMOTE_WAKEUP, and we can add another one: NO_POLL, use it like the following:
1 Set the NO_POLL pm qos flag when the underlying driver thinks it is no
longer necessary. In the ZPODD's case, it should be set when the device is to be powered off;
2 Clear it when poll is necessary again. In the ZPODD's case, when power
is re-gained, this flag will be cleared.

Quoted 13 lines

Quoted 2 lines

3 In the disk_events_workfn, check if this flag is set, if so, simply
return.

It should be, skip calling disk->fops->check_events, but still queue the work for next time's poll.

-Aaron

Quoted 5 lines

The disk->driverfs_dev can be used to host the pm qos flag, ATA layer can access it through ata_device->sdev->sdev_gendev.

Is this OK?

No, I don't think so. PM QoS is about telling the layer that will put the device into low-power states what states are to be taken into consideration. In this case, however, we need to tell someone else that the device has been turned off. Clearly, we need a way to do that, but not through PM QoS.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Did you consider using pm_runtime_suspended() to check the device status?

The problem is, a device can be in runtime suspended state while still needs to be polled...

Well, maybe this is the problem, then? Why does it need to be polled when suspended?

For ODDs, poll is not necessary only when ZP capable ODD is powered off. For other ODDs, poll still needs to go on.

ZP capable ODDs:
- runtime suspended, power remained(due to NO_POWER_OFF qos flag) poll is needed
-- runtime suspended, power removed
poll is not needed

Non ZP capable ODDs:
-- runtime suspended, power remained (power will never be removed)
poll is needed

If we do not poll for the powered on case, we will lose media change event.

Quoted 63 lines · Aaron Lu

Quoted 41 lines · Rafael J. Wysocki

Quoted 40 lines · Aaron Lu

Quoted 39 lines · Rafael J. Wysocki

Quoted 19 lines · Aaron Lu

Quoted 16 lines · Aaron Lu

Quoted 5 lines · Tejun Heo

I really think we need a way for (auto)pm and event polling to talk to each other so that autopm can tell event poll to sod off while pm is in effect. Trying to solve this from inside libata doesn't seem right. The problem, again, seems to be figuring out which hardware device maps to which block device. Hmmm... Any good ideas?

A possible way of doing this is using pm qos.

We currently have 2 pm qos flags, NO_POWER_OFF and REMOTE_WAKEUP, and we can add another one: NO_POLL, use it like the following:
1 Set the NO_POLL pm qos flag when the underlying driver thinks it is no
longer necessary. In the ZPODD's case, it should be set when the device is to be powered off;
2 Clear it when poll is necessary again. In the ZPODD's case, when power
is re-gained, this flag will be cleared.

Quoted 13 lines

Quoted 2 lines

3 In the disk_events_workfn, check if this flag is set, if so, simply
return.

It should be, skip calling disk->fops->check_events, but still queue the work for next time's poll.

-Aaron

Quoted 5 lines

The disk->driverfs_dev can be used to host the pm qos flag, ATA layer can access it through ata_device->sdev->sdev_gendev.

Is this OK?

No, I don't think so. PM QoS is about telling the layer that will put the device into low-power states what states are to be taken into consideration. In this case, however, we need to tell someone else that the device has been turned off. Clearly, we need a way to do that, but not through PM QoS.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Did you consider using pm_runtime_suspended() to check the device status?

The problem is, a device can be in runtime suspended state while still needs to be polled...

Well, maybe this is the problem, then? Why does it need to be polled when suspended?

For ODDs, poll is not necessary only when ZP capable ODD is powered off. For other ODDs, poll still needs to go on.

ZP capable ODDs:
- runtime suspended, power remained(due to NO_POWER_OFF qos flag) poll is needed
-- runtime suspended, power removed
poll is not needed

Non ZP capable ODDs:
-- runtime suspended, power remained (power will never be removed)
poll is needed

Quoted 2 lines

If we do not poll for the powered on case, we will lose media change event.

But the media change event should change the status from suspended to active, shouldn't it?

Rafael

Quoted 64 lines · Rafael J. Wysocki

Quoted 63 lines · Aaron Lu

Quoted 41 lines · Rafael J. Wysocki

Quoted 40 lines · Aaron Lu

Quoted 39 lines · Rafael J. Wysocki

Quoted 19 lines · Aaron Lu

Quoted 16 lines · Aaron Lu

Quoted 5 lines · Tejun Heo

I really think we need a way for (auto)pm and event polling to talk to each other so that autopm can tell event poll to sod off while pm is in effect. Trying to solve this from inside libata doesn't seem right. The problem, again, seems to be figuring out which hardware device maps to which block device. Hmmm... Any good ideas?

A possible way of doing this is using pm qos.

We currently have 2 pm qos flags, NO_POWER_OFF and REMOTE_WAKEUP, and we can add another one: NO_POLL, use it like the following:
1 Set the NO_POLL pm qos flag when the underlying driver thinks it is no
longer necessary. In the ZPODD's case, it should be set when the device is to be powered off;
2 Clear it when poll is necessary again. In the ZPODD's case, when power
is re-gained, this flag will be cleared.

Quoted 13 lines

Quoted 2 lines

3 In the disk_events_workfn, check if this flag is set, if so, simply
return.

It should be, skip calling disk->fops->check_events, but still queue the work for next time's poll.

-Aaron

Quoted 5 lines

The disk->driverfs_dev can be used to host the pm qos flag, ATA layer can access it through ata_device->sdev->sdev_gendev.

Is this OK?

No, I don't think so. PM QoS is about telling the layer that will put the device into low-power states what states are to be taken into consideration. In this case, however, we need to tell someone else that the device has been turned off. Clearly, we need a way to do that, but not through PM QoS.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Did you consider using pm_runtime_suspended() to check the device status?

The problem is, a device can be in runtime suspended state while still needs to be polled...

Well, maybe this is the problem, then? Why does it need to be polled when suspended?

For ODDs, poll is not necessary only when ZP capable ODD is powered off. For other ODDs, poll still needs to go on.

ZP capable ODDs:
- runtime suspended, power remained(due to NO_POWER_OFF qos flag) poll is needed
-- runtime suspended, power removed
poll is not needed

Non ZP capable ODDs:
-- runtime suspended, power remained (power will never be removed)
poll is needed

Quoted 5 lines

Quoted 2 lines

If we do not poll for the powered on case, we will lose media change event.

But the media change event should change the status from suspended to active, shouldn't it?

The media change event is derived from the poll, if we stop the poll, how can we know the event in the first place?

Quoted 71 lines · Aaron Lu

Quoted 69 lines · Rafael J. Wysocki

Quoted 59 lines · Aaron Lu

Quoted 41 lines · Rafael J. Wysocki

Quoted 40 lines · Aaron Lu

Quoted 39 lines · Rafael J. Wysocki

Quoted 19 lines · Aaron Lu

Quoted 16 lines · Aaron Lu

Quoted 5 lines · Tejun Heo

I really think we need a way for (auto)pm and event polling to talk to each other so that autopm can tell event poll to sod off while pm is in effect. Trying to solve this from inside libata doesn't seem right. The problem, again, seems to be figuring out which hardware device maps to which block device. Hmmm... Any good ideas?

A possible way of doing this is using pm qos.

We currently have 2 pm qos flags, NO_POWER_OFF and REMOTE_WAKEUP, and we can add another one: NO_POLL, use it like the following:
1 Set the NO_POLL pm qos flag when the underlying driver thinks it is no
longer necessary. In the ZPODD's case, it should be set when the device is to be powered off;
2 Clear it when poll is necessary again. In the ZPODD's case, when power
is re-gained, this flag will be cleared.

Quoted 13 lines

Quoted 2 lines

3 In the disk_events_workfn, check if this flag is set, if so, simply
return.

It should be, skip calling disk->fops->check_events, but still queue the work for next time's poll.

-Aaron

Quoted 5 lines

The disk->driverfs_dev can be used to host the pm qos flag, ATA layer can access it through ata_device->sdev->sdev_gendev.

Is this OK?

No, I don't think so. PM QoS is about telling the layer that will put the device into low-power states what states are to be taken into consideration. In this case, however, we need to tell someone else that the device has been turned off. Clearly, we need a way to do that, but not through PM QoS.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Did you consider using pm_runtime_suspended() to check the device status?

The problem is, a device can be in runtime suspended state while still needs to be polled...

Well, maybe this is the problem, then? Why does it need to be polled when suspended?

For ODDs, poll is not necessary only when ZP capable ODD is powered off. For other ODDs, poll still needs to go on.

ZP capable ODDs:
- runtime suspended, power remained(due to NO_POWER_OFF qos flag) poll is needed
-- runtime suspended, power removed
poll is not needed

Quoted 6 lines

Non ZP capable ODDs:
-- runtime suspended, power remained (power will never be removed)
poll is needed

If we do not poll for the powered on case, we will lose media change event.

But the media change event should change the status from suspended to active, shouldn't it?

Quoted 2 lines

The media change event is derived from the poll, if we stop the poll, how can we know the event in the first place?

OK, so what you're trying to say is that if the device is not turned off and the user opens the tray and inserts a media in there, we won't know that that happened without polling. Is that correct?

If so, can you please remind me why we want to pretend that the device is suspended if we want to poll it?

Rafael

Quoted 63 lines · Aaron Lu

Quoted 41 lines · Rafael J. Wysocki

Quoted 40 lines · Aaron Lu

Quoted 39 lines · Rafael J. Wysocki

Quoted 19 lines · Aaron Lu

Quoted 16 lines · Aaron Lu

Quoted 5 lines · Tejun Heo

I really think we need a way for (auto)pm and event polling to talk to each other so that autopm can tell event poll to sod off while pm is in effect. Trying to solve this from inside libata doesn't seem right. The problem, again, seems to be figuring out which hardware device maps to which block device. Hmmm... Any good ideas?

A possible way of doing this is using pm qos.

We currently have 2 pm qos flags, NO_POWER_OFF and REMOTE_WAKEUP, and we can add another one: NO_POLL, use it like the following:
1 Set the NO_POLL pm qos flag when the underlying driver thinks it is no
longer necessary. In the ZPODD's case, it should be set when the device is to be powered off;
2 Clear it when poll is necessary again. In the ZPODD's case, when power
is re-gained, this flag will be cleared.

Quoted 13 lines

Quoted 2 lines

3 In the disk_events_workfn, check if this flag is set, if so, simply
return.

It should be, skip calling disk->fops->check_events, but still queue the work for next time's poll.

-Aaron

Quoted 5 lines

The disk->driverfs_dev can be used to host the pm qos flag, ATA layer can access it through ata_device->sdev->sdev_gendev.

Is this OK?

No, I don't think so. PM QoS is about telling the layer that will put the device into low-power states what states are to be taken into consideration. In this case, however, we need to tell someone else that the device has been turned off. Clearly, we need a way to do that, but not through PM QoS.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Did you consider using pm_runtime_suspended() to check the device status?

The problem is, a device can be in runtime suspended state while still needs to be polled...

Well, maybe this is the problem, then? Why does it need to be polled when suspended?

For ODDs, poll is not necessary only when ZP capable ODD is powered off. For other ODDs, poll still needs to go on.

ZP capable ODDs:
- runtime suspended, power remained(due to NO_POWER_OFF qos flag) poll is needed
-- runtime suspended, power removed
poll is not needed

Non ZP capable ODDs:
-- runtime suspended, power remained (power will never be removed)
poll is needed

Quoted 2 lines

If we do not poll for the powered on case, we will lose media change event.

But the media change event should change the status from suspended to active, shouldn't it?

Rafael

A new interface to block disk events is added, this interface is meant to eliminate a race between PM runtime callback and disk events checking.

Suppose the following device tree:
device_sata_port (the parent)
device_ODD (the child)

Quoted 4 lines · Aaron Lu

A new interface to block disk events is added, this interface is meant to eliminate a race between PM runtime callback and disk events checking.

Quoted 3 lines

Suppose the following device tree:
device_sata_port (the parent)
device_ODD (the child)

Weren't you gonna do something different about this? I mean, if sr knows that autopm kicked in, it can simply tell the block layer that nothing is going on. Wouldn't that be simpler?

Quoted 3 lines

Weren't you gonna do something different about this? I mean, if sr knows that autopm kicked in, it can simply tell the block layer that nothing is going on. Wouldn't that be simpler?

It wouldn't work for non-ZP drives. They do need to be polled, even when suspended.

Hello, Alan.

Quoted 4 lines · Alan Stern

Quoted 3 lines

Weren't you gonna do something different about this? I mean, if sr knows that autopm kicked in, it can simply tell the block layer that nothing is going on. Wouldn't that be simpler?

Quoted 2 lines

It wouldn't work for non-ZP drives. They do need to be polled, even when suspended.

Hmmm... a bit confused, what would autopm do for those non-ZP devices? Would it make any difference?

It wouldn't do anything for the device, but it would allow the device's parent to go to low power in between polls. That's why Aaron at one point suggested lengthening the polling interval.

Hello, Alan.

Quoted 3 lines

It wouldn't do anything for the device, but it would allow the device's parent to go to low power in between polls. That's why Aaron at one point suggested lengthening the polling interval.

Ah, okay. Entering and leaving suspend even every two seconds may save noticeable power for some controllers. Does that, tho, mean that the port would get reset every two seconds?

If so (I can't see how
else it would behave), it may not really work. I mean, you would be spending about ten seconds reinitiaizing everything and then enter sleep and do it all over again two seconds later, most likely wasting power in the process. Sounds weird to me. What am I missing?

Quoted 5 lines

If so (I can't see how
else it would behave), it may not really work. I mean, you would be spending about ten seconds reinitiaizing everything and then enter sleep and do it all over again two seconds later, most likely wasting power in the process. Sounds weird to me. What am I missing?

Does it really take 10 seconds to recover from an ATA suspend? That sounds awfully long.

The two second value is merely the default interval for media polling. Devices with no async notification mechanism require some sort of polling if anybody wants to know when media is inserted or removed, and the polling can't be carried out if the device's port is suspended.

You asked about autopm. "Autopm" is a rather vague and ugly term I made up; it means that the kernel automatically suspends the device after some specified idle time. In the case of optical drives, "idle" currently means the device file is not open (which implies the drive doesn't contain a mounted filesystem). As far as I know, the kernel has no direct way to affect the power level of an optical drive that isn't ZP. For these devices "suspended" is merely a logical state, meaning that the device isn't in use and therefore its parent can be put into a low-power mode.

Regardless, an optical drive can't remain suspended when polling occurs. So unless there's some other mechanism for getting the information about media changes, suspended drives (and their parents) will have to be resumed periodically for polling.

Hence there's a tradeoff. How can we use the minimum amount of energy while still polling the drive acceptably often? In general, the kernel doesn't know. That's why these things can be controlled from userspace. And the answer may be different for ATA drives vs. USB-connected drives.

Does this answer your questions?

I think the only reason autopm doesn't blow up for SATA devices is that userland usually automatically mounts them thus effectively disabling autopm. I *think* the only sane thing we can do is doing autopm iff zpodd is available && no media.

Quoted 2 lines

Does it really take 10 seconds to recover from an ATA suspend? That sounds awfully long.

If it's using the same ->suspend ops as the regular system suspend, 10secs isn't a crazy number. If the controller goes offline, the PHY would go offline too and the only way to come back from that is performing full probing sequence. From SATA procotol POV, it isn't too bad. It's just link initiazliation followed by IDENTIFY and some config commands.

The problem is that SATA devices aren't really designed to be used like that. If you reset a ODD w/ a media in it, it'll probably spin it up and try to re-establish media state during probe sequence. It isn't designed that way and has never been used like that. SATA has its own dynamic link power management (DIPM/HIPM) tho.

So, this whole autopm thing doesn't sound like a good idea to me.

Quoted 7 lines

Hence there's a tradeoff. How can we use the minimum amount of energy while still polling the drive acceptably often? In general, the kernel doesn't know. That's why these things can be controlled from userspace. And the answer may be different for ATA drives vs. USB-connected drives.

Does this answer your questions?

I think the only reason autopm doesn't blow up for SATA devices is that userland usually automatically mounts them thus effectively disabling autopm.

I *think* the only sane thing we can do is doing
autopm iff zpodd is available && no media.

Quoted 5 lines

The problem is that SATA devices aren't really designed to be used like that. If you reset a ODD w/ a media in it, it'll probably spin it up and try to re-establish media state during probe sequence. It isn't designed that way and has never been used like that. SATA has its own dynamic link power management (DIPM/HIPM) tho.

Is it possible to use those to implement runtime suspend? Or are they handled autonomously by the hardware?

Quoted 1 line

So, this whole autopm thing doesn't sound like a good idea to me.

No doubt it's better suited to some devices than to others.

Quoted 3 lines

I think the only reason autopm doesn't blow up for SATA devices is that userland usually automatically mounts them thus effectively disabling autopm.

Either that or else because it hasn't been fully implemented yet. :-)

Quoted 2 lines

I *think* the only sane thing we can do is doing
autopm iff zpodd is available && no media.

That may be true for SATA. For USB optical drives, it does make sense to power-down the host controller when the drive isn't in use. USB suspend/resume takes on the order of 50-100 ms or so.

Hey, Alan.

It's controlled by /sys/class/scsi_host/hostX/link_power_management. Once enabled, the power saving is completedly handled by hardware. If link stays idle longer than certain duration, the hardware initiates low power state and leaves it when something needs to happen.

Quoted 3 lines

Quoted 1 line

So, this whole autopm thing doesn't sound like a good idea to me.

No doubt it's better suited to some devices than to others.

Yeah, SATA seems to need a different approach than USB.

Quoted 3 lines

That may be true for SATA. For USB optical drives, it does make sense to power-down the host controller when the drive isn't in use. USB suspend/resume takes on the order of 50-100 ms or so.

I see. For SATA too, the controller / link bring up doesn't take too long. The crappy part is what the attached, especially ATAPI, devices would do after such events as those events will be visible to them basically as random link resets.

So, at least for SATA, I think what autopm can do is...

* Trigger zpodd if possible.

* Trigger suspend iff polling isn't happening on the device.

Quoted 1 line · Tejun Heo

Hey, Alan.

Hey...

Quoted 10 lines

It's controlled by /sys/class/scsi_host/hostX/link_power_management. Once enabled, the power saving is completedly handled by hardware. If link stays idle longer than certain duration, the hardware initiates low power state and leaves it when something needs to happen.

Quoted 3 lines

Quoted 1 line

So, this whole autopm thing doesn't sound like a good idea to me.

No doubt it's better suited to some devices than to others.

Yeah, SATA seems to need a different approach than USB.

Based on your description, I agree.

Quoted 13 lines

Quoted 3 lines

That may be true for SATA. For USB optical drives, it does make sense to power-down the host controller when the drive isn't in use. USB suspend/resume takes on the order of 50-100 ms or so.

I see. For SATA too, the controller / link bring up doesn't take too long. The crappy part is what the attached, especially ATAPI, devices would do after such events as those events will be visible to them basically as random link resets.

So, at least for SATA, I think what autopm can do is...

* Trigger zpodd if possible.

Quoted 1 line

* Trigger suspend iff polling isn't happening on the device.

That sums it up nicely. Of course, the PM core is unaware of details such as media polling. What we can do is have the SATA runtime-idle method return -EBUSY if the device isn't a ZPODD and if polling is enabled. Unfortunately I don't think there's any way currently to trigger autopm when the user turns off polling. Maybe something could be added to the appropriate sysfs handler.

Hey...

Quoted 4 lines

So, at least for SATA, I think what autopm can do is...

* Trigger zpodd if possible.

Quoted 1 line

* Trigger suspend iff polling isn't happening on the device.

That sums it up nicely. Of course, the PM core is unaware of details such as media polling. What we can do is have the SATA runtime-idle method return -EBUSY if the device isn't a ZPODD and if polling is enabled. Unfortunately I don't think there's any way currently to trigger autopm when the user turns off polling. Maybe something could be added to the appropriate sysfs handler.