Re: [PATCH 3/3] usb: Change persist_enabled when attribute avoid_reset_quirk is modified
On 2012年8月2日 14:44:12, Oliver Neukum wrote: On Wednesday 01 August 2012 23:14:03 Lan Tianyu wrote: On 2012/8/1 22:48, Oliver Neukum wrote: Strictly speaking, no. It is possible that there are devices that may do something bad in case of a reset, but sometimes work well. In such cases we should try. Do you mean some devices of this kind will not morph after reset? I a little confuse since in my mind that all such kind devices will morph after reset. The point is that we don't know. User space has set a flag for reasons of its own. If user space wants to clear the persist attribute it can do so. Ok. I get it. Regards Oliver -- Best Regards Tianyu Lan linux kernel enabling team -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] usb: Change persist_enabled when attribute avoid_reset_quirk is modified
On Wednesday 01 August 2012 23:14:03 Lan Tianyu wrote: > On 2012/8/1 22:48, Oliver Neukum wrote: > > Strictly speaking, no. It is possible that there are devices that may > > do something bad in case of a reset, but sometimes work well. In > > such cases we should try. > Do you mean some devices of this kind will not morph after reset? I a > little confuse since in my mind that all such kind devices will morph > after reset. The point is that we don't know. User space has set a flag for reasons of its own. If user space wants to clear the persist attribute it can do so. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] usb: Change persist_enabled when attribute avoid_reset_quirk is modified
On Wed, 1 Aug 2012, Lan Tianyu wrote: > On 2012/7/31 22:39, Alan Stern wrote: > > On Tue, 31 Jul 2012, Lan Tianyu wrote: > > > >> How about checking RESET_MORPHS before doing reset_resume, set reset_resume > >> to 0 and do resume when RESET_MORPHS is set. > > > > No, that won't work. When we do a reset-resume it is because we _know_ > > that a regular resume will fail. > persist is default to be enabled. When user space set a device's > avoid_reset_quirk, they also should set persist to 0. Since these > devices can be reset and regular resume can work. Right? It doesn't really matter. Yes, regular resume can work. And reset-resume will fail, regardless of the persist setting. > > I think the best course is to leave things the way they are. Just add > > an explanation to persist.txt that the Persist mechanism is likely to > > fail if the avoid_reset_quirk attribute is set. > > If we leave things the way they are, do we still need previous patch > "Take attribute avoid_reset_quirk out of usb device's attribute group"? No, we don't. And it doesn't need to do anything special for hubs. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] usb: Change persist_enabled when attribute avoid_reset_quirk is modified
On 2012/8/1 22:48, Oliver Neukum wrote: On Wednesday 01 August 2012 22:46:51 Lan Tianyu wrote: No, that won't work. When we do a reset-resume it is because we know that a regular resume will fail. persist is default to be enabled. When user space set a device's avoid_reset_quirk, they also should set persist to 0. Since these devices can be reset and regular resume can work. Right? Strictly speaking, no. It is possible that there are devices that may do something bad in case of a reset, but sometimes work well. In such cases we should try. Do you mean some devices of this kind will not morph after reset? I a little confuse since in my mind that all such kind devices will morph after reset. Regards Oliver -- Best Regards Tianyu Lan linux kernel enabling team -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] usb: Change persist_enabled when attribute avoid_reset_quirk is modified
On Wednesday 01 August 2012 22:46:51 Lan Tianyu wrote: > > No, that won't work. When we do a reset-resume it is because we know > > that a regular resume will fail. > persist is default to be enabled. When user space set a device's > avoid_reset_quirk, they also should set persist to 0. Since these > devices can be reset and regular resume can work. Right? Strictly speaking, no. It is possible that there are devices that may do something bad in case of a reset, but sometimes work well. In such cases we should try. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] usb: Change persist_enabled when attribute avoid_reset_quirk is modified
On 2012/7/31 22:39, Alan Stern wrote: On Tue, 31 Jul 2012, Lan Tianyu wrote: How about checking RESET_MORPHS before doing reset_resume, set reset_resume to 0 and do resume when RESET_MORPHS is set. No, that won't work. When we do a reset-resume it is because we _know_ that a regular resume will fail. persist is default to be enabled. When user space set a device's avoid_reset_quirk, they also should set persist to 0. Since these devices can be reset and regular resume can work. Right? At the same time, print "Convert reset_resume to resume due to RESET_MORPHS". Then these two attributes are separated but for reset-resume, there are two conditions. persist is true RESET_MORPHS is unset. I think the best course is to leave things the way they are. Just add an explanation to persist.txt that the Persist mechanism is likely to fail if the avoid_reset_quirk attribute is set. If we leave things the way they are, do we still need previous patch "Take attribute avoid_reset_quirk out of usb device's attribute group"? Alan Stern -- Best Regards Tianyu Lan linux kernel enabling team -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] usb: Change persist_enabled when attribute avoid_reset_quirk is modified
On Tue, 31 Jul 2012, Lan Tianyu wrote: > How about checking RESET_MORPHS before doing reset_resume, set reset_resume > to 0 and do resume when RESET_MORPHS is set. No, that won't work. When we do a reset-resume it is because we _know_ that a regular resume will fail. > At the same time, print "Convert > reset_resume to resume due to RESET_MORPHS". Then these two attributes are > separated but for reset-resume, there are two conditions. persist is true > RESET_MORPHS is unset. I think the best course is to leave things the way they are. Just add an explanation to persist.txt that the Persist mechanism is likely to fail if the avoid_reset_quirk attribute is set. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] usb: Change persist_enabled when attribute avoid_reset_quirk is modified
On 2012年07月30日 23:16, Alan Stern wrote: On Mon, 30 Jul 2012, Oliver Neukum wrote: On Monday 30 July 2012 10:39:50 Alan Stern wrote: this leads to behavior that is not easy to predict or understand. It would be cleaner to leave the flag alone and check for the quirk in addition for the flag when the check is done. I disagree. Leaving the flag set but then not implementing the persist behavior would be confusing also. Maybe even worse. But we cannot guarantee persistance anyway. We can only try. But trying makes no sense if we are sure to fail. In addition, we even undermine the file permission to a small extent if we allow one attribute to alter another attribute. Putting all these thoughts together leads to a single conclusion: We should not change persist_enabled when avoid_reset_quirk gets set. As you say, coupling the two attributes is confusing and circumvents the permissions. If the device needs a reset-resume, we'll go ahead and try to do it. If the reset fails because the firmware gets erased, at least there will be an error message in the system log to explain what went wrong. (But it's still a good idea to add a sentence about this issue to the Documentation file.) Whereas if we silently change attribute values or silently skip a reset-resume when RESET_MORPHS is set, there will be no indication in the log or anywhere else to tell the user what happened. How about checking RESET_MORPHS before doing reset_resume, set reset_resume to 0 and do resume when RESET_MORPHS is set. At the same time, print "Convert reset_resume to resume due to RESET_MORPHS". Then these two attributes are separated but for reset-resume, there are two conditions. persist is true RESET_MORPHS is unset. -- Best Regards Tianyu Lan linux kernel enabling team -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] usb: Change persist_enabled when attribute avoid_reset_quirk is modified
On Monday 30 July 2012 11:16:12 Alan Stern wrote: > As you say, coupling the two attributes is confusing and circumvents > the permissions. If the device needs a reset-resume, we'll go ahead > and try to do it. If the reset fails because the firmware gets erased, > at least there will be an error message in the system log to explain > what went wrong. (But it's still a good idea to add a sentence about > this issue to the Documentation file.) > > Whereas if we silently change attribute values or silently skip a > reset-resume when RESET_MORPHS is set, there will be no indication in > the log or anywhere else to tell the user what happened. That makes sense. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] usb: Change persist_enabled when attribute avoid_reset_quirk is modified
On Mon, 30 Jul 2012, Oliver Neukum wrote: > On Monday 30 July 2012 10:39:50 Alan Stern wrote: > > > this leads to behavior that is not easy to predict or understand. > > > It would be cleaner to leave the flag alone and check for the quirk > > > in addition for the flag when the check is done. > > > > I disagree. Leaving the flag set but then not implementing the > > persist behavior would be confusing also. Maybe even worse. > > But we cannot guarantee persistance anyway. We can only try. > But trying makes no sense if we are sure to fail. > > In addition, we even undermine the file permission to a small > extent if we allow one attribute to alter another attribute. Putting all these thoughts together leads to a single conclusion: We should not change persist_enabled when avoid_reset_quirk gets set. As you say, coupling the two attributes is confusing and circumvents the permissions. If the device needs a reset-resume, we'll go ahead and try to do it. If the reset fails because the firmware gets erased, at least there will be an error message in the system log to explain what went wrong. (But it's still a good idea to add a sentence about this issue to the Documentation file.) Whereas if we silently change attribute values or silently skip a reset-resume when RESET_MORPHS is set, there will be no indication in the log or anywhere else to tell the user what happened. Besides, as far as I can tell the RESET_MORPHS quirk is used in only one place (in usb-storage). We don't even have any existing quirk entries that set this flag! Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] usb: Change persist_enabled when attribute avoid_reset_quirk is modified
On Monday 30 July 2012 10:39:50 Alan Stern wrote: > > this leads to behavior that is not easy to predict or understand. > > It would be cleaner to leave the flag alone and check for the quirk > > in addition for the flag when the check is done. > > I disagree. Leaving the flag set but then not implementing the > persist behavior would be confusing also. Maybe even worse. But we cannot guarantee persistance anyway. We can only try. But trying makes no sense if we are sure to fail. In addition, we even undermine the file permission to a small extent if we allow one attribute to alter another attribute. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] usb: Change persist_enabled when attribute avoid_reset_quirk is modified
On Mon, 30 Jul 2012, Oliver Neukum wrote: > On Monday 30 July 2012 15:58:25 Lan Tianyu wrote: > > On 2012年07月30日 15:24, Oliver Neukum wrote: > > > > this is on second thought quite problematic. If user space has > > > disabled the persist feature it must stay disabled, even if > > > the quirk setting is removed. > > Yeah. You are right. So how about remain the persist_enabled 0 or > > do nothing with persist_enabled when quirk settting is removed. > > Hi, > > this leads to behavior that is not easy to predict or understand. > It would be cleaner to leave the flag alone and check for the quirk > in addition for the flag when the check is done. I disagree. Leaving the flag set but then not implementing the persist behavior would be confusing also. Maybe even worse. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] usb: Change persist_enabled when attribute avoid_reset_quirk is modified
On Mon, 30 Jul 2012, Lan Tianyu wrote: > The attribute avoid_reset_quirk means the device should not be reset. > when it is set, persist_enabled also should be set to 0 to prevent > reset-resume when the device resumes. > > Current only in the usb_detect_quirks(), persist_enabled will > be set depending on whether the dev's flag USB_QUIRK_RESET_MORPHS > is set or not. And usb_detect_quirks() is only called in the > hub_port_connect_change() when a new device is found. So after a > device being enumerated, Changing attribute avod_reset_quirk > will not set persist_enabled to 0 to prevent reset-resume. > > This patch is to change persist_enabled when attribute avoid_reset_quirk > is modified. When attribute avoid_reset_quirk is set, attribute persist > should be unmoidified and remains 0 since attribute avoid_reset_quirk > means not able to be reset. So this patch also adds USB_QUIRK_RESET_MORPHS > check before changing persist_enabled in the attribute persist callback(). > > Signed-off-by: Lan Tianyu > --- > Documentation/usb/persist.txt |5 - > drivers/usb/core/sysfs.c | 10 +++--- > 2 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/Documentation/usb/persist.txt b/Documentation/usb/persist.txt > index 074b159..0e287d6 100644 > --- a/Documentation/usb/persist.txt > +++ b/Documentation/usb/persist.txt > @@ -107,7 +107,10 @@ where the "..." should be filled in the with the > device's ID. Disable > the feature by writing 0 instead of 1. For hubs the feature is > automatically and permanently enabled and the power/persist file > doesn't even exist, so you only have to worry about setting it for > -devices where it really matters. > +devices where it really matters. When attribute avoid_reset_quirk > +is set, the persist can't be changed and remains 0 since > +avoid_reset_quirk means the device can't be reset and reset-resume > +doesn't work for the device. You should avoid mentioning things the reader won't understand. The term "reset-resume" is not used in persist.txt, and you shouldn't introduce it like this. Just end the sentence after "can't be reset". > @@ -311,7 +314,8 @@ set_persist(struct device *dev, struct device_attribute > *attr, > return -EINVAL; > > usb_lock_device(udev); > - udev->persist_enabled = !!value; > + if (!(udev->quirks & USB_QUIRK_RESET_MORPHS)) > + udev->persist_enabled = !!value; If USB_QUIRK_RESET_MORPHS is set, you should return an error code such as -EINVAL. > usb_unlock_device(udev); > return count; > } Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] usb: Change persist_enabled when attribute avoid_reset_quirk is modified
On Monday 30 July 2012 15:58:25 Lan Tianyu wrote: > On 2012年07月30日 15:24, Oliver Neukum wrote: > > this is on second thought quite problematic. If user space has > > disabled the persist feature it must stay disabled, even if > > the quirk setting is removed. > Yeah. You are right. So how about remain the persist_enabled 0 or > do nothing with persist_enabled when quirk settting is removed. Hi, this leads to behavior that is not easy to predict or understand. It would be cleaner to leave the flag alone and check for the quirk in addition for the flag when the check is done. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] usb: Change persist_enabled when attribute avoid_reset_quirk is modified
On 2012年07月30日 15:24, Oliver Neukum wrote:
On Monday 30 July 2012 11:34:11 Lan Tianyu wrote:
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -209,10 +209,13 @@ set_avoid_reset_quirk(struct device *dev, struct
device_attribute *attr,
if (sscanf(buf, "%d",&val) != 1 || val< 0 || val> 1)
return -EINVAL;
usb_lock_device(udev);
- if (val)
+ if (val) {
udev->quirks |= USB_QUIRK_RESET_MORPHS;
- else
+ udev->persist_enabled = 0;
+ } else {
udev->quirks&= ~USB_QUIRK_RESET_MORPHS;
+ udev->persist_enabled = 1;
+ }
usb_unlock_device(udev);
return count;
Hi,
this is on second thought quite problematic. If user space has
disabled the persist feature it must stay disabled, even if
the quirk setting is removed.
Yeah. You are right. So how about remain the persist_enabled 0 or
do nothing with persist_enabled when quirk settting is removed.
Regards
Oliver
--
Best Regards
Tianyu Lan
linux kernel enabling team
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] usb: Change persist_enabled when attribute avoid_reset_quirk is modified
On Monday 30 July 2012 11:34:11 Lan Tianyu wrote:
> --- a/drivers/usb/core/sysfs.c
> +++ b/drivers/usb/core/sysfs.c
> @@ -209,10 +209,13 @@ set_avoid_reset_quirk(struct device *dev, struct
> device_attribute *attr,
> if (sscanf(buf, "%d", &val) != 1 || val < 0 || val > 1)
> return -EINVAL;
> usb_lock_device(udev);
> - if (val)
> + if (val) {
> udev->quirks |= USB_QUIRK_RESET_MORPHS;
> - else
> + udev->persist_enabled = 0;
> + } else {
> udev->quirks &= ~USB_QUIRK_RESET_MORPHS;
> + udev->persist_enabled = 1;
> + }
> usb_unlock_device(udev);
> return count;
Hi,
this is on second thought quite problematic. If user space has
disabled the persist feature it must stay disabled, even if
the quirk setting is removed.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] usb: Change persist_enabled when attribute avoid_reset_quirk is modified
The attribute avoid_reset_quirk means the device should not be reset.
when it is set, persist_enabled also should be set to 0 to prevent
reset-resume when the device resumes.
Current only in the usb_detect_quirks(), persist_enabled will
be set depending on whether the dev's flag USB_QUIRK_RESET_MORPHS
is set or not. And usb_detect_quirks() is only called in the
hub_port_connect_change() when a new device is found. So after a
device being enumerated, Changing attribute avod_reset_quirk
will not set persist_enabled to 0 to prevent reset-resume.
This patch is to change persist_enabled when attribute avoid_reset_quirk
is modified. When attribute avoid_reset_quirk is set, attribute persist
should be unmoidified and remains 0 since attribute avoid_reset_quirk
means not able to be reset. So this patch also adds USB_QUIRK_RESET_MORPHS
check before changing persist_enabled in the attribute persist callback().
Signed-off-by: Lan Tianyu
---
Documentation/usb/persist.txt |5 -
drivers/usb/core/sysfs.c | 10 +++---
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/Documentation/usb/persist.txt b/Documentation/usb/persist.txt
index 074b159..0e287d6 100644
--- a/Documentation/usb/persist.txt
+++ b/Documentation/usb/persist.txt
@@ -107,7 +107,10 @@ where the "..." should be filled in the with the device's
ID. Disable
the feature by writing 0 instead of 1. For hubs the feature is
automatically and permanently enabled and the power/persist file
doesn't even exist, so you only have to worry about setting it for
-devices where it really matters.
+devices where it really matters. When attribute avoid_reset_quirk
+is set, the persist can't be changed and remains 0 since
+avoid_reset_quirk means the device can't be reset and reset-resume
+doesn't work for the device.
Is this the best solution?
diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index 20b0add..37b6367 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -209,10 +209,13 @@ set_avoid_reset_quirk(struct device *dev, struct
device_attribute *attr,
if (sscanf(buf, "%d", &val) != 1 || val < 0 || val > 1)
return -EINVAL;
usb_lock_device(udev);
- if (val)
+ if (val) {
udev->quirks |= USB_QUIRK_RESET_MORPHS;
- else
+ udev->persist_enabled = 0;
+ } else {
udev->quirks &= ~USB_QUIRK_RESET_MORPHS;
+ udev->persist_enabled = 1;
+ }
usb_unlock_device(udev);
return count;
}
@@ -311,7 +314,8 @@ set_persist(struct device *dev, struct device_attribute
*attr,
return -EINVAL;
usb_lock_device(udev);
- udev->persist_enabled = !!value;
+ if (!(udev->quirks & USB_QUIRK_RESET_MORPHS))
+ udev->persist_enabled = !!value;
usb_unlock_device(udev);
return count;
}
--
1.7.6.rc2.8.g28eb
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
