Re: [PATCH 5/8] [media] em28xx: initial support for HAUPPAUGE HVR-930C again
On 05-12-2011 21:47, Devin Heitmueller wrote:
On Mon, Dec 5, 2011 at 6:32 PM, Eddi De Pierie...@depieri.net wrote:
Sorry, I think I applied follow patch on my tree while I developed
the driver trying to fix tuner initialization.
http://patchwork.linuxtv.org/patch/6617/
I forgot to remove from my tree after I see that don't solve anything.
Ok, great. At least that explains why it's there (since I couldn't
figure out how on Earth the patch made sense otherwise).
Eddi, could you please submit a patch removing the offending code?
Ok, As Eddi agreed with this change, I'm adding the enclosed patch to
the development tree, removing the bad code.
I'll do a quick test before pushing it upstream.
Regards,
Mauro
-
[media] xc5000: Remove the global mutex lock at xc5000 firmware init
As reported by Devin Heitmueller dheitmuel...@kernellabs.com:
It seems like a change such as this could significantly change the
timing of tuner initialization if you have multiple xc5000 based
products that might have a slow i2c bus. Was that intentional?
After discussed with Eddi de Pierri e...@depieri.net, it was pointed that
the change was not intentional, and it was just a trial while developing
the patches that add support for HVR-930C.
So, remove this hack.
Reported-by: Devin Heitmueller dheitmuel...@kernellabs.com
Acked by: Eddi de Pierri e...@depieri.net
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
diff --git a/drivers/media/common/tuners/xc5000.c
b/drivers/media/common/tuners/xc5000.c
index 048f489..ecd1f95 100644
--- a/drivers/media/common/tuners/xc5000.c
+++ b/drivers/media/common/tuners/xc5000.c
@@ -1004,8 +1004,6 @@ static int xc_load_fw_and_init_tuner(struct dvb_frontend
*fe)
struct xc5000_priv *priv = fe-tuner_priv;
int ret = 0;
- mutex_lock(xc5000_list_mutex);
-
if (xc5000_is_firmware_loaded(fe) != XC_RESULT_SUCCESS) {
ret = xc5000_fwupload(fe);
if (ret != XC_RESULT_SUCCESS)
@@ -1025,8 +1023,6 @@ static int xc_load_fw_and_init_tuner(struct dvb_frontend
*fe)
/* Default to CABLE mode */
ret |= xc_write_reg(priv, XREG_SIGNALSOURCE, XC_RF_MODE_CABLE);
- mutex_unlock(xc5000_list_mutex);
-
return ret;
}
Thanks,
Devin
--
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: [PATCH 5/8] [media] em28xx: initial support for HAUPPAUGE HVR-930C again
On 11-12-05 06:47 PM, Devin Heitmueller wrote: On Mon, Dec 5, 2011 at 6:32 PM, Eddi De Pieri e...@depieri.net wrote: Sorry, I think I applied follow patch on my tree while I developed the driver trying to fix tuner initialization. http://patchwork.linuxtv.org/patch/6617/ I forgot to remove from my tree after I see that don't solve anything. Ok, great. At least that explains why it's there (since I couldn't figure out how on Earth the patch made sense otherwise). Eddi, could you please submit a patch removing the offending code? That's good. But there definitely still is a race between modules in there somewhere. The HVR-950Q tuners use several: xc5000, au8522, au0828, .. and unless au0828 is loaded *last*, with a delay before/after, the dongles don't always work. Preloading all of the modules before allowing hardware detection seems to help. Even just changing from a mechanical hard drive to a very fast SSD is enough to change the behaviour from not-working to working (and sometimes the other way around). I tried to track this down a couple of years ago, and found cross-module calls failing because the target functions hadn't been loaded yet. But my lack of notes from 2-3 years ago isn't helpful here. Here's a similar report from 2 years ago, as valid today as it was then: http://www.mythtv.org/pipermail/mythtv-users/2010-January/279912.html Cheers -- 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: [PATCH 5/8] [media] em28xx: initial support for HAUPPAUGE HVR-930C again
On 06-12-2011 10:51, Mark Lord wrote: On 11-12-05 06:47 PM, Devin Heitmueller wrote: On Mon, Dec 5, 2011 at 6:32 PM, Eddi De Pierie...@depieri.net wrote: Sorry, I think I applied follow patch on my tree while I developed the driver trying to fix tuner initialization. http://patchwork.linuxtv.org/patch/6617/ I forgot to remove from my tree after I see that don't solve anything. Ok, great. At least that explains why it's there (since I couldn't figure out how on Earth the patch made sense otherwise). Eddi, could you please submit a patch removing the offending code? That's good. But there definitely still is a race between modules in there somewhere. The HVR-950Q tuners use several: xc5000, au8522, au0828, .. and unless au0828 is loaded *last*, with a delay before/after, the dongles don't always work. Preloading all of the modules before allowing hardware detection seems to help. Even just changing from a mechanical hard drive to a very fast SSD is enough to change the behaviour from not-working to working (and sometimes the other way around). I tried to track this down a couple of years ago, and found cross-module calls failing because the target functions hadn't been loaded yet. But my lack of notes from 2-3 years ago isn't helpful here. Here's a similar report from 2 years ago, as valid today as it was then: http://www.mythtv.org/pipermail/mythtv-users/2010-January/279912.html The driver who binds everything is the bridge driver. In your case, it is the au0828 driver. What you're experiencing seems to be some race issue inside it, and not at xc5000. On a quick look on it, I'm noticing that there's no lock at au0828_usb_probe(). Also, it uses a separate lock for analog and for digital: mutex_init(dev-mutex); mutex_init(dev-dvb.lock); Probably, the right thing to do would be to use just one lock for both rising it at usb_probe, lowering it just before return 0. This will avoid any open operations while the device is not fully initialized. Btw, newer udev's open the analog part of the driver just after V4L register, in order to get the device capabilities. This is known to cause race conditions, if the locking schema is not working properly. Regards, 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: [PATCH 5/8] [media] em28xx: initial support for HAUPPAUGE HVR-930C again
On Tue, Dec 6, 2011 at 8:43 AM, Mauro Carvalho Chehab mche...@redhat.com wrote: The driver who binds everything is the bridge driver. In your case, it is the au0828 driver. What you're experiencing seems to be some race issue inside it, and not at xc5000. On a quick look on it, I'm noticing that there's no lock at au0828_usb_probe(). Also, it uses a separate lock for analog and for digital: mutex_init(dev-mutex); mutex_init(dev-dvb.lock); Probably, the right thing to do would be to use just one lock for both rising it at usb_probe, lowering it just before return 0. This will avoid any open operations while the device is not fully initialized. Btw, newer udev's open the analog part of the driver just after V4L register, in order to get the device capabilities. This is known to cause race conditions, if the locking schema is not working properly. Just to be clear, we're now talking about a completely different race condition that has nothing to do with the subject at hand, and this discussion should probably be moved to a new thread. That said, yes, there is definitely a race (if not two) in there to be tracked down. I know of a couple of users who upgraded to more recent kernels and started experiencing breakage on module load where there was none before. This could obviously be dumb luck in that perhaps the timing changed slightly, or it could be some change in the core code which created a new race. I haven't had the time/energy to dig into the issue (compounded by the fact that these sorts of issues are notoriously difficult to debug when it cannot be reproduced locally by the developer). The notion that this is something that has been there for over a year is something I only learned of in the last couple of days. All the complaints I had seen thus far were from existing users who were perfectly happy until they upgraded their kernel a couple of months ago and then started seeing the problem. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- 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: [PATCH 5/8] [media] em28xx: initial support for HAUPPAUGE HVR-930C again
On 11-12-06 08:56 AM, Devin Heitmueller wrote: On Tue, Dec 6, 2011 at 8:43 AM, Mauro Carvalho Chehab mche...@redhat.com wrote: The driver who binds everything is the bridge driver. In your case, it is the au0828 driver. What you're experiencing seems to be some race issue inside it, and not at xc5000. On a quick look on it, I'm noticing that there's no lock at au0828_usb_probe(). Also, it uses a separate lock for analog and for digital: mutex_init(dev-mutex); mutex_init(dev-dvb.lock); Probably, the right thing to do would be to use just one lock for both rising it at usb_probe, lowering it just before return 0. This will avoid any open operations while the device is not fully initialized. Btw, newer udev's open the analog part of the driver just after V4L register, in order to get the device capabilities. This is known to cause race conditions, if the locking schema is not working properly. Just to be clear, we're now talking about a completely different race condition that has nothing to do with the subject at hand, and this discussion should probably be moved to a new thread. If this discussion does change threads, could you folks please copy me on it? I'm already subscribed to several other kernel mailing lists in my roles as developer and maintainer of various bits, but I would like to avoid having yet another daily deluge added to my inbox. :) That said, I can test possible fixes for this stuff, and am rather interested to see it resolved. .. The notion that this is something that has been there for over a year is something I only learned of in the last couple of days. All the complaints I had seen thus far were from existing users who were perfectly happy until they upgraded their kernel a couple of months ago and then started seeing the problem. .. It's always exhibited races for me here. I have long since worked around the issue(s), so my own systems currently behave. But with the newer HVR-950Q revision (B4F0), the issue is far more prevalent than before. I may try Mauro's locking suggestion -- more detail or a patch would be useful. 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: [PATCH 5/8] [media] em28xx: initial support for HAUPPAUGE HVR-930C again
On Tue, Dec 6, 2011 at 10:28 AM, Mark Lord ker...@teksavvy.com wrote: It's always exhibited races for me here. I have long since worked around the issue(s), so my own systems currently behave. But with the newer HVR-950Q revision (B4F0), the issue is far more prevalent than before. I'll ask around and see if I can find out what they changed in the B4F0 revision. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- 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: [PATCH 5/8] [media] em28xx: initial support for HAUPPAUGE HVR-930C again
On 06-12-2011 13:28, Mark Lord wrote: On 11-12-06 08:56 AM, Devin Heitmueller wrote: On Tue, Dec 6, 2011 at 8:43 AM, Mauro Carvalho Chehab mche...@redhat.com wrote: The driver who binds everything is the bridge driver. In your case, it is the au0828 driver. What you're experiencing seems to be some race issue inside it, and not at xc5000. On a quick look on it, I'm noticing that there's no lock at au0828_usb_probe(). Also, it uses a separate lock for analog and for digital: mutex_init(dev-mutex); mutex_init(dev-dvb.lock); Probably, the right thing to do would be to use just one lock for both rising it at usb_probe, lowering it just before return 0. This will avoid any open operations while the device is not fully initialized. Btw, newer udev's open the analog part of the driver just after V4L register, in order to get the device capabilities. This is known to cause race conditions, if the locking schema is not working properly. Just to be clear, we're now talking about a completely different race condition that has nothing to do with the subject at hand, and this discussion should probably be moved to a new thread. If this discussion does change threads, could you folks please copy me on it? I'm already subscribed to several other kernel mailing lists in my roles as developer and maintainer of various bits, but I would like to avoid having yet another daily deluge added to my inbox. :) That said, I can test possible fixes for this stuff, and am rather interested to see it resolved. .. The notion that this is something that has been there for over a year is something I only learned of in the last couple of days. All the complaints I had seen thus far were from existing users who were perfectly happy until they upgraded their kernel a couple of months ago and then started seeing the problem. .. It's always exhibited races for me here. I have long since worked around the issue(s), so my own systems currently behave. But with the newer HVR-950Q revision (B4F0), the issue is far more prevalent than before. I may try Mauro's locking suggestion -- more detail or a patch would be useful. You may take a look at the lock changes applied on em28xx driver for some examples. You basically need to block access to DVB while the device is handling a V4L syscall and vice-versa. Changing the locking schema is not trivial, as it may generate dead locks. So, careful testing is required. It also helps to compile a kernel with the dead lock detection logic enabled, as it may help you to discover if you did something wrong. Regards, Mauro 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: [PATCH 5/8] [media] em28xx: initial support for HAUPPAUGE HVR-930C again
On Sun, Nov 20, 2011 at 9:56 AM, Mauro Carvalho Chehab
mche...@redhat.com wrote:
diff --git a/drivers/media/common/tuners/xc5000.c
b/drivers/media/common/tuners/xc5000.c
index ecd1f95..048f489 100644
--- a/drivers/media/common/tuners/xc5000.c
+++ b/drivers/media/common/tuners/xc5000.c
@@ -1004,6 +1004,8 @@ static int xc_load_fw_and_init_tuner(struct
dvb_frontend *fe)
struct xc5000_priv *priv = fe-tuner_priv;
int ret = 0;
+ mutex_lock(xc5000_list_mutex);
+
if (xc5000_is_firmware_loaded(fe) != XC_RESULT_SUCCESS) {
ret = xc5000_fwupload(fe);
if (ret != XC_RESULT_SUCCESS)
@@ -1023,6 +1025,8 @@ static int xc_load_fw_and_init_tuner(struct
dvb_frontend *fe)
/* Default to CABLE mode */
ret |= xc_write_reg(priv, XREG_SIGNALSOURCE, XC_RF_MODE_CABLE);
+ mutex_unlock(xc5000_list_mutex);
+
return ret;
}
What's up with this change? Is this a bugfix for some race condition?
Why is it jammed into a patch for some particular product?
It seems like a change such as this could significantly change the
timing of tuner initialization if you have multiple xc5000 based
products that might have a slow i2c bus. Was that intentional?
This patch should be NACK'd and resubmitted as it's own bugfix where
it's implications can be fully understood in the context of all the
other products that use xc5000.
Devin
--
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: [PATCH 5/8] [media] em28xx: initial support for HAUPPAUGE HVR-930C again
On 05-12-2011 16:23, Devin Heitmueller wrote:
On Sun, Nov 20, 2011 at 9:56 AM, Mauro Carvalho Chehab
mche...@redhat.com wrote:
diff --git a/drivers/media/common/tuners/xc5000.c
b/drivers/media/common/tuners/xc5000.c
index ecd1f95..048f489 100644
--- a/drivers/media/common/tuners/xc5000.c
+++ b/drivers/media/common/tuners/xc5000.c
@@ -1004,6 +1004,8 @@ static int xc_load_fw_and_init_tuner(struct dvb_frontend
*fe)
struct xc5000_priv *priv = fe-tuner_priv;
int ret = 0;
+ mutex_lock(xc5000_list_mutex);
+
if (xc5000_is_firmware_loaded(fe) != XC_RESULT_SUCCESS) {
ret = xc5000_fwupload(fe);
if (ret != XC_RESULT_SUCCESS)
@@ -1023,6 +1025,8 @@ static int xc_load_fw_and_init_tuner(struct dvb_frontend
*fe)
/* Default to CABLE mode */
ret |= xc_write_reg(priv, XREG_SIGNALSOURCE, XC_RF_MODE_CABLE);
+ mutex_unlock(xc5000_list_mutex);
+
return ret;
}
What's up with this change? Is this a bugfix for some race condition?
Why is it jammed into a patch for some particular product?
It seems like a change such as this could significantly change the
timing of tuner initialization if you have multiple xc5000 based
products that might have a slow i2c bus. Was that intentional?
This patch should be NACK'd and resubmitted as it's own bugfix where
it's implications can be fully understood in the context of all the
other products that use xc5000.
It is too late for nacking the patch, as there are several other patches
were already applied on the top of it, and we don't rebase the
linux-media.git tree.
Assuming that this is due to some bug that Eddi picked during xc5000
init, what it can be done now is to write a patch that would replace
this xc5000-global mutex lock into a some other per-device locking
schema.
Regards,
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: [PATCH 5/8] [media] em28xx: initial support for HAUPPAUGE HVR-930C again
On Mon, Dec 5, 2011 at 1:35 PM, Mauro Carvalho Chehab mche...@redhat.com wrote: What's up with this change? Is this a bugfix for some race condition? Why is it jammed into a patch for some particular product? It seems like a change such as this could significantly change the timing of tuner initialization if you have multiple xc5000 based products that might have a slow i2c bus. Was that intentional? This patch should be NACK'd and resubmitted as it's own bugfix where it's implications can be fully understood in the context of all the other products that use xc5000. It is too late for nacking the patch, as there are several other patches were already applied on the top of it, and we don't rebase the linux-media.git tree. Assuming that this is due to some bug that Eddi picked during xc5000 init, what it can be done now is to write a patch that would replace this xc5000-global mutex lock into a some other per-device locking schema. At this point we have zero idea why it's there *at all*. Eddi, can you comment on what prompted this change? This patch should not have been accepted in the first place. It's an undocumented change on a different driver than is advertised in the subject line. Did you review the patch prior to merging? This change can result in a performance regression for all other devices using xc5000, and it's not yet clear why it's there in the first place. If its use cannot be explained then it should be rolled back. If this breaks 930c, then the whole device support series should be rolled back until somebody can figure out what is going on. It's crap like this that is the reason that every other week I get complaints from some user that one of the drivers I wrote support for worked fine for months/years until they upgraded to the latest kernel. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- 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: [PATCH 5/8] [media] em28xx: initial support for HAUPPAUGE HVR-930C again
On Mon, Dec 5, 2011 at 1:46 PM, Devin Heitmueller dheitmuel...@kernellabs.com wrote: On Mon, Dec 5, 2011 at 1:35 PM, Mauro Carvalho Chehab mche...@redhat.com wrote: What's up with this change? Is this a bugfix for some race condition? Why is it jammed into a patch for some particular product? It seems like a change such as this could significantly change the timing of tuner initialization if you have multiple xc5000 based products that might have a slow i2c bus. Was that intentional? This patch should be NACK'd and resubmitted as it's own bugfix where it's implications can be fully understood in the context of all the other products that use xc5000. It is too late for nacking the patch, as there are several other patches were already applied on the top of it, and we don't rebase the linux-media.git tree. Assuming that this is due to some bug that Eddi picked during xc5000 init, what it can be done now is to write a patch that would replace this xc5000-global mutex lock into a some other per-device locking schema. At this point we have zero idea why it's there *at all*. Eddi, can you comment on what prompted this change? This patch should not have been accepted in the first place. It's an undocumented change on a different driver than is advertised in the subject line. Did you review the patch prior to merging? This change can result in a performance regression for all other devices using xc5000, and it's not yet clear why it's there in the first place. If its use cannot be explained then it should be rolled back. If this breaks 930c, then the whole device support series should be rolled back until somebody can figure out what is going on. It's crap like this that is the reason that every other week I get complaints from some user that one of the drivers I wrote support for worked fine for months/years until they upgraded to the latest kernel. Speaking of which, Mark Lord just tried out this change (he has an 800i and 950q - both xc5000 based), and now his DVB stack fails to load. And yes, he already has the fix to the mutex_unlock() regression which Dan Carpenter found six days ago and which this patch introduced. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- 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: [PATCH 5/8] [media] em28xx: initial support for HAUPPAUGE HVR-930C again
Sorry, I think I applied follow patch on my tree while I developed the driver trying to fix tuner initialization. http://patchwork.linuxtv.org/patch/6617/ I forgot to remove from my tree after I see that don't solve anything. Regards Eddi On Mon, Dec 5, 2011 at 9:01 PM, Devin Heitmueller dheitmuel...@kernellabs.com wrote: On Mon, Dec 5, 2011 at 1:46 PM, Devin Heitmueller dheitmuel...@kernellabs.com wrote: On Mon, Dec 5, 2011 at 1:35 PM, Mauro Carvalho Chehab mche...@redhat.com wrote: What's up with this change? Is this a bugfix for some race condition? Why is it jammed into a patch for some particular product? It seems like a change such as this could significantly change the timing of tuner initialization if you have multiple xc5000 based products that might have a slow i2c bus. Was that intentional? This patch should be NACK'd and resubmitted as it's own bugfix where it's implications can be fully understood in the context of all the other products that use xc5000. It is too late for nacking the patch, as there are several other patches were already applied on the top of it, and we don't rebase the linux-media.git tree. Assuming that this is due to some bug that Eddi picked during xc5000 init, what it can be done now is to write a patch that would replace this xc5000-global mutex lock into a some other per-device locking schema. At this point we have zero idea why it's there *at all*. Eddi, can you comment on what prompted this change? This patch should not have been accepted in the first place. It's an undocumented change on a different driver than is advertised in the subject line. Did you review the patch prior to merging? This change can result in a performance regression for all other devices using xc5000, and it's not yet clear why it's there in the first place. If its use cannot be explained then it should be rolled back. If this breaks 930c, then the whole device support series should be rolled back until somebody can figure out what is going on. It's crap like this that is the reason that every other week I get complaints from some user that one of the drivers I wrote support for worked fine for months/years until they upgraded to the latest kernel. Speaking of which, Mark Lord just tried out this change (he has an 800i and 950q - both xc5000 based), and now his DVB stack fails to load. And yes, he already has the fix to the mutex_unlock() regression which Dan Carpenter found six days ago and which this patch introduced. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- 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: [PATCH 5/8] [media] em28xx: initial support for HAUPPAUGE HVR-930C again
On Mon, Dec 5, 2011 at 6:32 PM, Eddi De Pieri e...@depieri.net wrote: Sorry, I think I applied follow patch on my tree while I developed the driver trying to fix tuner initialization. http://patchwork.linuxtv.org/patch/6617/ I forgot to remove from my tree after I see that don't solve anything. Ok, great. At least that explains why it's there (since I couldn't figure out how on Earth the patch made sense otherwise). Eddi, could you please submit a patch removing the offending code? Thanks, Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- 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
