Re: [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2-patches
On Fri, 21 May 2010, Mauro Carvalho Chehab wrote: > Mike Isely wrote: [snip] > > The point when the kernel started complaining about the use of a stack > > based USB I/O buffers is the relevant point, which was not back in > > 2.6.12. I learned of this behavior (that is, receiving warnings about > > the usage) as being new in the 2.6.34 timeframe, the point when a user > > pointed out the complaint message in his kernel log; at that time I had > > not yet tested against that kernel version. > > Older kernels also complain if the stack were actually out of the > DMA range and you try to program DMA there. Yet, only now we've seen > consumer PC's with lots of RAM. One of my test targets is a PC in 32 bit mode with 4GB of RAM; strange then that I've never seen the kernel complain there. The bad buffer has been in the driver for even longer than that and nobody raised the issue to me until about a month ago (the fix was created back then but for other reasons that you already know it didn't become available in -hg until last week). > > > Leave it as is. What's done is done. Your replaced comment is of > > course still correct. I would just appreciate some better sensitivity > > in the future about replacing authors' comments, especially since in > > this case your interpretation of my original comment was wrong. > > Ok. Thanks. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2-patches
Mike Isely wrote: > On Fri, 21 May 2010, Mauro Carvalho Chehab wrote: > >> Mike Isely wrote: >>> On Fri, 21 May 2010, Mauro Carvalho Chehab wrote: >>> Mike Isely wrote: > Mauro: > > You are reading too much into that comment. > > I never said it was valid to do what had been done, only that for the > longest time this is what the driver did and it never caused a problem > that I was made aware of. What I said there was correct, that this is > what the driver had been doing in the past, that it's definitely causing > a problem now and thus that is why this patch exists. As I said, this is not right: "Apparently later kernels don't like this behavior" >>> Mauro: >>> >>> That statement was in reference to the fact that previously the problem >>> had gone undetected, but now later kernels can notice and complain about >>> this, thus "later kernels don't like this behavior". >>> >>> We can debate that perhaps the statement can be worded better, but that >>> doesn't make it *wrong*. >>> >> Calling 2.6.12 kernel as "later kernels" doesn't seem right to me (that was >> about the kernel were em28xx driver were introduced). > > The point when the em28xx driver appeared has nothing to do with this. > > The point when the kernel started complaining about the use of a stack > based USB I/O buffers is the relevant point, which was not back in > 2.6.12. I learned of this behavior (that is, receiving warnings about > the usage) as being new in the 2.6.34 timeframe, the point when a user > pointed out the complaint message in his kernel log; at that time I had > not yet tested against that kernel version. Older kernels also complain if the stack were actually out of the DMA range and you try to program DMA there. Yet, only now we've seen consumer PC's with lots of RAM. > Leave it as is. What's done is done. Your replaced comment is of > course still correct. I would just appreciate some better sensitivity > in the future about replacing authors' comments, especially since in > this case your interpretation of my original comment was wrong. Ok. -- Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2-patches
On Fri, 21 May 2010, Mauro Carvalho Chehab wrote: > Mike Isely wrote: > > On Fri, 21 May 2010, Mauro Carvalho Chehab wrote: > > > >> Mike Isely wrote: > >>> Mauro: > >>> > >>> You are reading too much into that comment. > >>> > >>> I never said it was valid to do what had been done, only that for the > >>> longest time this is what the driver did and it never caused a problem > >>> that I was made aware of. What I said there was correct, that this is > >>> what the driver had been doing in the past, that it's definitely causing > >>> a problem now and thus that is why this patch exists. > >> As I said, this is not right: > >>"Apparently later kernels don't like this behavior" > > > > Mauro: > > > > That statement was in reference to the fact that previously the problem > > had gone undetected, but now later kernels can notice and complain about > > this, thus "later kernels don't like this behavior". > > > > We can debate that perhaps the statement can be worded better, but that > > doesn't make it *wrong*. > > > > Calling 2.6.12 kernel as "later kernels" doesn't seem right to me (that was > about the kernel were em28xx driver were introduced). The point when the em28xx driver appeared has nothing to do with this. The point when the kernel started complaining about the use of a stack based USB I/O buffers is the relevant point, which was not back in 2.6.12. I learned of this behavior (that is, receiving warnings about the usage) as being new in the 2.6.34 timeframe, the point when a user pointed out the complaint message in his kernel log; at that time I had not yet tested against that kernel version. > > > > >> It is not "later kernels". DMA over stack were never supported. Your driver > >> had a bug that you didn't noticed for long time, probably because nobody > >> reported you this issue, since it appears only on some non-Intel archs and > >> on i386 with more than 3.12 Gb of RAM, and when the stack happens to be > >> after > >> the first 3.12 Gb (with is a somewhat rare condition). > > > > I understand your point perfectly that this was never right or valid. > > In fact, I also understood that point long before you decided to explain > > it to me here - after all my realization of this problem in the driver > > is why I wrote the patch in the first place. Absolutely no argument > > there about the importance of the change. > > > > None of that however justifies putting words into an author's mouth, > > which is effectively what you did by replacing that commit comment. > > > > First of all, it is clearly stated at the patch that the description > were changed and by whom: > [mche...@redhat.com: fix patch description] > > Second, it is an usual upstream practice to fix descriptions where needed. > The patch description is a precious resource for people that are seeking > for similar bugs, and for those that are trying to understand some code. > So, it is important to not send broken/incomplete comments to kernel, > or comments that may have a dubious interpretation. So, subsystem maintainers > frequently need to fix comments. > > Anyway, to end this discussion, I can simply revert the patch from the > staging tree, waiting for a new patch from you with a fixed comment. > > Also, if you prefer, next time, I won't apply any patch from you if I > found a bad comment without your ack, even if it means that you'll > probably loose a merge window. Leave it as is. What's done is done. Your replaced comment is of course still correct. I would just appreciate some better sensitivity in the future about replacing authors' comments, especially since in this case your interpretation of my original comment was wrong. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2-patches
Mike Isely wrote: > On Fri, 21 May 2010, Mauro Carvalho Chehab wrote: > >> Mike Isely wrote: >>> Mauro: >>> >>> You are reading too much into that comment. >>> >>> I never said it was valid to do what had been done, only that for the >>> longest time this is what the driver did and it never caused a problem >>> that I was made aware of. What I said there was correct, that this is >>> what the driver had been doing in the past, that it's definitely causing >>> a problem now and thus that is why this patch exists. >> As I said, this is not right: >> "Apparently later kernels don't like this behavior" > > Mauro: > > That statement was in reference to the fact that previously the problem > had gone undetected, but now later kernels can notice and complain about > this, thus "later kernels don't like this behavior". > > We can debate that perhaps the statement can be worded better, but that > doesn't make it *wrong*. > Calling 2.6.12 kernel as "later kernels" doesn't seem right to me (that was about the kernel were em28xx driver were introduced). > >> It is not "later kernels". DMA over stack were never supported. Your driver >> had a bug that you didn't noticed for long time, probably because nobody >> reported you this issue, since it appears only on some non-Intel archs and >> on i386 with more than 3.12 Gb of RAM, and when the stack happens to be after >> the first 3.12 Gb (with is a somewhat rare condition). > > I understand your point perfectly that this was never right or valid. > In fact, I also understood that point long before you decided to explain > it to me here - after all my realization of this problem in the driver > is why I wrote the patch in the first place. Absolutely no argument > there about the importance of the change. > > None of that however justifies putting words into an author's mouth, > which is effectively what you did by replacing that commit comment. > First of all, it is clearly stated at the patch that the description were changed and by whom: [mche...@redhat.com: fix patch description] Second, it is an usual upstream practice to fix descriptions where needed. The patch description is a precious resource for people that are seeking for similar bugs, and for those that are trying to understand some code. So, it is important to not send broken/incomplete comments to kernel, or comments that may have a dubious interpretation. So, subsystem maintainers frequently need to fix comments. Anyway, to end this discussion, I can simply revert the patch from the staging tree, waiting for a new patch from you with a fixed comment. Also, if you prefer, next time, I won't apply any patch from you if I found a bad comment without your ack, even if it means that you'll probably loose a merge window. -- Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2-patches
On Fri, 21 May 2010, Mauro Carvalho Chehab wrote: > Mike Isely wrote: > > Mauro: > > > > You are reading too much into that comment. > > > > I never said it was valid to do what had been done, only that for the > > longest time this is what the driver did and it never caused a problem > > that I was made aware of. What I said there was correct, that this is > > what the driver had been doing in the past, that it's definitely causing > > a problem now and thus that is why this patch exists. > > As I said, this is not right: > "Apparently later kernels don't like this behavior" Mauro: That statement was in reference to the fact that previously the problem had gone undetected, but now later kernels can notice and complain about this, thus "later kernels don't like this behavior". We can debate that perhaps the statement can be worded better, but that doesn't make it *wrong*. > > It is not "later kernels". DMA over stack were never supported. Your driver > had a bug that you didn't noticed for long time, probably because nobody > reported you this issue, since it appears only on some non-Intel archs and > on i386 with more than 3.12 Gb of RAM, and when the stack happens to be after > the first 3.12 Gb (with is a somewhat rare condition). I understand your point perfectly that this was never right or valid. In fact, I also understood that point long before you decided to explain it to me here - after all my realization of this problem in the driver is why I wrote the patch in the first place. Absolutely no argument there about the importance of the change. None of that however justifies putting words into an author's mouth, which is effectively what you did by replacing that commit comment. When I've propagated commits from other authors who have sent me patches, I try very hard to ensure that their comments are preserved verbatim. Those are after all the author's words not mine. If I think such a comment has a problem, then I will discuss it with the author and let him/her provide the replacement statement (or at least explain to me why (s)he thinks it is correct). Failing that then I will at least make it clear in the comment which edits came from me. I would never want to be accused of unilaterally putting words into peoples' mouths and I would hope others will treat me the same way. -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2-patches
Mike Isely wrote:
> Mauro:
>
> You are reading too much into that comment.
>
> I never said it was valid to do what had been done, only that for the
> longest time this is what the driver did and it never caused a problem
> that I was made aware of. What I said there was correct, that this is
> what the driver had been doing in the past, that it's definitely causing
> a problem now and thus that is why this patch exists.
As I said, this is not right:
"Apparently later kernels don't like this behavior"
It is not "later kernels". DMA over stack were never supported. Your driver
had a bug that you didn't noticed for long time, probably because nobody
reported you this issue, since it appears only on some non-Intel archs and
on i386 with more than 3.12 Gb of RAM, and when the stack happens to be after
the first 3.12 Gb (with is a somewhat rare condition).
If you take a look at the old logs, you'll see, at commit
a6c2ba283565dbc9f055dcb2ecba1971460bb535 (nov, 2005) the fix on one of the
drivers:
+int em2820_write_regs_req(struct em2820 *dev, u8 req, u16 reg, char *buf,
+ int len)
+{
+ int ret;
+
+ /*usb_control_msg seems to expect a kmalloced buffer */
+ unsigned char *bufs = kmalloc(len, GFP_KERNEL);
The same bug hit me in 2006/2007 when writing tm6000 driver. Due to a problem
at the videobuf free logic, on that time, it were very frequent to hit this
bug, as, with memory being spent, eventually stack address would be above the
3Gb address.
Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2-patches
Mauro: You are reading too much into that comment. I never said it was valid to do what had been done, only that for the longest time this is what the driver did and it never caused a problem that I was made aware of. What I said there was correct, that this is what the driver had been doing in the past, that it's definitely causing a problem now and thus that is why this patch exists. I'd really rather you not mess with my comment. Probably too late however. -Mike On Fri, 21 May 2010, Mauro Carvalho Chehab wrote: > Mike Isely wrote: > > Please from http://linuxtv.org/hg/~mcisely/pvrusb2-patches for the > > following pvrusb2 driver fixes / improvements: > > > > - pvrusb2: Minor debug code fixup > > - pvrusb2: Fix Gotview hardware support > > - pvrusb2: Avoid using stack allocated buffers when performing USB I/O > > Your comment for this patch is wrong: > > pvrusb2: The pvrusb2 driver has for the longest time used a (tiny) > stack allocated buffer for some of its I/O with the hardware. > Apparently later kernels don't like this behavior and trap it at > run-time, causing nasty complaints to the kernel log. This trivial > change fixes the one case in the driver where this had been happening. > > It were never valid to use stack for DMA, as kernel provides > no warranty that the stack would be on a page that can do DMA. > In a matter of fact, as most x86 USB drivers accept DMA at the first > 3Gb of RAM space, this bug is generally not noticed on i386/x86_64 > archs. Yet, if your machine has more than 3Gb, there are some chances that > the stack would be at the HIGHMEM area, where DMA is not supported by the > processor. > > As this is a common error, newer kernels have some instrumentation support to > warn about such troubles. > > I'll be fixing the comment. > > > -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2-patches
Mike Isely wrote: > Please from http://linuxtv.org/hg/~mcisely/pvrusb2-patches for the > following pvrusb2 driver fixes / improvements: > > - pvrusb2: Minor debug code fixup > - pvrusb2: Fix Gotview hardware support > - pvrusb2: Avoid using stack allocated buffers when performing USB I/O Your comment for this patch is wrong: pvrusb2: The pvrusb2 driver has for the longest time used a (tiny) stack allocated buffer for some of its I/O with the hardware. Apparently later kernels don't like this behavior and trap it at run-time, causing nasty complaints to the kernel log. This trivial change fixes the one case in the driver where this had been happening. It were never valid to use stack for DMA, as kernel provides no warranty that the stack would be on a page that can do DMA. In a matter of fact, as most x86 USB drivers accept DMA at the first 3Gb of RAM space, this bug is generally not noticed on i386/x86_64 archs. Yet, if your machine has more than 3Gb, there are some chances that the stack would be at the HIGHMEM area, where DMA is not supported by the processor. As this is a common error, newer kernels have some instrumentation support to warn about such troubles. I'll be fixing the comment. -- Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PULL] http://linuxtv.org/hg/~mcisely/pvrusb2-patches
Please from http://linuxtv.org/hg/~mcisely/pvrusb2-patches for the following pvrusb2 driver fixes / improvements: - pvrusb2: Minor debug code fixup - pvrusb2: Fix Gotview hardware support - pvrusb2: Avoid using stack allocated buffers when performing USB I/O - pvrusb2: New feature to mark specific hardware support as experimental - pvrusb2: Fix kernel oops at device unregistration - pvrusb2: Fix missing header include - pvrusb2: Fix USB parent device reference count - pvrusb2: Fix minor internal array allocation - pvrusb2: Fix kernel oops on device tear-down - pvrusb2: Call sysfs_attr_init() appropriately... pvrusb2-devattr.c |1 pvrusb2-devattr.h |5 pvrusb2-hdw.c | 26 + pvrusb2-main.c|4 +-- pvrusb2-sysfs.c | 64 +++--- pvrusb2-v4l2.c| 16 ++--- 6 files changed, 107 insertions(+), 9 deletions(-) These are primarily a collection of stability fixes. Thanks, -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PULL] http://linuxtv.org/hg/~mcisely/pvrusb2-patches
Please from http://linuxtv.org/hg/~mcisely/pvrusb2-patches for the following pvrusb2 driver fixes / improvements: - pvrusb2: Enforce a 300msec stabilization interval during stream strart - pvrusb2: Reduce encoder quiet period - pvrusb2: Adjust 300msec digitizer wait to be more selective pvrusb2-hdw-internal.h | 12 - pvrusb2-hdw.c | 61 - pvrusb2-hdw.h |1 3 files changed, 61 insertions(+), 13 deletions(-) These fixes improve mpeg streaming startup stability for any pvrusb2-driven device which contains an saa7115 video digitizer. Thanks, -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
