| Message ID | 1530128483-31662-2-git-send-email-brad@nextdimension.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Apologies, forgot to note this patch requires: https://patchwork.kernel.org/patch/10492053/ On 2018-06-27 14:41, Brad Love wrote: > Addresses the following, which introduced a regression itself: > > Commit 509f89652f83 ("media: em28xx: fix a regression with HVR-950") > > The regression fix breaks dual transport stream support. When a > tuner starts streaming it sets alt mode on the USB interface. > The problem is both tuners share the same USB interface, so when > the second tuner becomes active and sets alt mode on the interface > it kills streaming on the other port. > > It was suggested to add a refcounter and only set alt mode if no > tuners are currently active on the interface. This requires > sharing some state amongst both tuner devices, with appropriate > locking. > > What I've done here is the following: > - Add a usage_count pointer to struct em28xx > - Share usage_count between both em28xx devices > - Only set alt mode if usage_count is zero > - Increment usage_count when each tuner becomes active > - Decrement usage_count when a tuner becomes idle > > With usage_count in the main em28xx struct, locking is handled as > follows: > - if a secondary tuner exists, lock dev->dev_next->lock > - if no secondary tuner exists, lock dev->lock > > By using the above scheme a single tuner device, will lock itself, > the first tuner in a dual tuner device will lock the second tuner, > and the second tuner in a dual tuner device will lock itself aka > the second tuner instance. > > Signed-off-by: Brad Love <brad@nextdimension.cc> > --- > drivers/media/usb/em28xx/em28xx-cards.c | 6 ++++- > drivers/media/usb/em28xx/em28xx-dvb.c | 47 +++++++++++++++++++++++++++++++-- > drivers/media/usb/em28xx/em28xx.h | 1 + > 3 files changed, 51 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c > index 48bc505..91f9787 100644 > --- a/drivers/media/usb/em28xx/em28xx-cards.c > +++ b/drivers/media/usb/em28xx/em28xx-cards.c > @@ -3374,8 +3374,10 @@ void em28xx_free_device(struct kref *ref) > if (!dev->disconnected) > em28xx_release_resources(dev); > > - if (dev->ts == PRIMARY_TS) > + if (dev->ts == PRIMARY_TS) { > kfree(dev->alt_max_pkt_size_isoc); > + kfree(dev->usage_count); > + } > > kfree(dev); > } > @@ -3793,6 +3795,8 @@ static int em28xx_usb_probe(struct usb_interface *intf, > dev->is_audio_only = has_vendor_audio && !(has_video || has_dvb); > dev->has_video = has_video; > dev->ifnum = ifnum; > + dev->usage_count = kcalloc(1, sizeof(unsigned int), GFP_KERNEL); > + *dev->usage_count = 0; > > dev->ts = PRIMARY_TS; > snprintf(dev->name, 28, "em28xx"); > diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c > index b778d8a..3a67831 100644 > --- a/drivers/media/usb/em28xx/em28xx-dvb.c > +++ b/drivers/media/usb/em28xx/em28xx-dvb.c > @@ -218,7 +218,19 @@ static int em28xx_start_streaming(struct em28xx_dvb *dvb) > dvb_alt = dev->dvb_alt_isoc; > } > > - usb_set_interface(udev, dev->ifnum, dvb_alt); > + if (dev->dev_next) > + mutex_lock(&dev->dev_next->lock); > + else > + mutex_lock(&dev->lock); > + > + if (*dev->usage_count == 0) > + usb_set_interface(udev, dev->ifnum, dvb_alt); > + > + if (dev->dev_next) > + mutex_unlock(&dev->dev_next->lock); > + else > + mutex_unlock(&dev->lock); > + > rc = em28xx_set_mode(dev, EM28XX_DIGITAL_MODE); > if (rc < 0) > return rc; > @@ -250,6 +262,8 @@ static int em28xx_start_feed(struct dvb_demux_feed *feed) > { > struct dvb_demux *demux = feed->demux; > struct em28xx_dvb *dvb = demux->priv; > + struct em28xx_i2c_bus *i2c_bus = dvb->adapter.priv; > + struct em28xx *dev = i2c_bus->dev; > int rc, ret; > > if (!demux->dmx.frontend) > @@ -263,6 +277,19 @@ static int em28xx_start_feed(struct dvb_demux_feed *feed) > ret = em28xx_start_streaming(dvb); > if (ret < 0) > rc = ret; > + else { > + if (dev->dev_next) > + mutex_lock(&dev->dev_next->lock); > + else > + mutex_lock(&dev->lock); > + > + *dev->usage_count = *dev->usage_count + 1; > + > + if (dev->dev_next) > + mutex_unlock(&dev->dev_next->lock); > + else > + mutex_unlock(&dev->lock); > + } > } > > mutex_unlock(&dvb->lock); > @@ -273,14 +300,30 @@ static int em28xx_stop_feed(struct dvb_demux_feed *feed) > { > struct dvb_demux *demux = feed->demux; > struct em28xx_dvb *dvb = demux->priv; > + struct em28xx_i2c_bus *i2c_bus = dvb->adapter.priv; > + struct em28xx *dev = i2c_bus->dev; > int err = 0; > > mutex_lock(&dvb->lock); > dvb->nfeeds--; > > - if (!dvb->nfeeds) > + if (!dvb->nfeeds) { > err = em28xx_stop_streaming(dvb); > > + if (dev->dev_next) > + mutex_lock(&dev->dev_next->lock); > + else > + mutex_lock(&dev->lock); > + > + *dev->usage_count = *dev->usage_count - 1; > + > + if (dev->dev_next) > + mutex_unlock(&dev->dev_next->lock); > + else > + mutex_unlock(&dev->lock); > + } > + > + > mutex_unlock(&dvb->lock); > return err; > } > diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h > index 953caac..d245c4f 100644 > --- a/drivers/media/usb/em28xx/em28xx.h > +++ b/drivers/media/usb/em28xx/em28xx.h > @@ -775,6 +775,7 @@ struct em28xx { > > struct em28xx *dev_next; > int ts; > + unsigned int *usage_count; > }; > > #define kref_to_dev(d) container_of(d, struct em28xx, ref)
diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c index 48bc505..91f9787 100644 --- a/drivers/media/usb/em28xx/em28xx-cards.c +++ b/drivers/media/usb/em28xx/em28xx-cards.c @@ -3374,8 +3374,10 @@ void em28xx_free_device(struct kref *ref) if (!dev->disconnected) em28xx_release_resources(dev); - if (dev->ts == PRIMARY_TS) + if (dev->ts == PRIMARY_TS) { kfree(dev->alt_max_pkt_size_isoc); + kfree(dev->usage_count); + } kfree(dev); } @@ -3793,6 +3795,8 @@ static int em28xx_usb_probe(struct usb_interface *intf, dev->is_audio_only = has_vendor_audio && !(has_video || has_dvb); dev->has_video = has_video; dev->ifnum = ifnum; + dev->usage_count = kcalloc(1, sizeof(unsigned int), GFP_KERNEL); + *dev->usage_count = 0; dev->ts = PRIMARY_TS; snprintf(dev->name, 28, "em28xx"); diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c index b778d8a..3a67831 100644 --- a/drivers/media/usb/em28xx/em28xx-dvb.c +++ b/drivers/media/usb/em28xx/em28xx-dvb.c @@ -218,7 +218,19 @@ static int em28xx_start_streaming(struct em28xx_dvb *dvb) dvb_alt = dev->dvb_alt_isoc; } - usb_set_interface(udev, dev->ifnum, dvb_alt); + if (dev->dev_next) + mutex_lock(&dev->dev_next->lock); + else + mutex_lock(&dev->lock); + + if (*dev->usage_count == 0) + usb_set_interface(udev, dev->ifnum, dvb_alt); + + if (dev->dev_next) + mutex_unlock(&dev->dev_next->lock); + else + mutex_unlock(&dev->lock); + rc = em28xx_set_mode(dev, EM28XX_DIGITAL_MODE); if (rc < 0) return rc; @@ -250,6 +262,8 @@ static int em28xx_start_feed(struct dvb_demux_feed *feed) { struct dvb_demux *demux = feed->demux; struct em28xx_dvb *dvb = demux->priv; + struct em28xx_i2c_bus *i2c_bus = dvb->adapter.priv; + struct em28xx *dev = i2c_bus->dev; int rc, ret; if (!demux->dmx.frontend) @@ -263,6 +277,19 @@ static int em28xx_start_feed(struct dvb_demux_feed *feed) ret = em28xx_start_streaming(dvb); if (ret < 0) rc = ret; + else { + if (dev->dev_next) + mutex_lock(&dev->dev_next->lock); + else + mutex_lock(&dev->lock); + + *dev->usage_count = *dev->usage_count + 1; + + if (dev->dev_next) + mutex_unlock(&dev->dev_next->lock); + else + mutex_unlock(&dev->lock); + } } mutex_unlock(&dvb->lock); @@ -273,14 +300,30 @@ static int em28xx_stop_feed(struct dvb_demux_feed *feed) { struct dvb_demux *demux = feed->demux; struct em28xx_dvb *dvb = demux->priv; + struct em28xx_i2c_bus *i2c_bus = dvb->adapter.priv; + struct em28xx *dev = i2c_bus->dev; int err = 0; mutex_lock(&dvb->lock); dvb->nfeeds--; - if (!dvb->nfeeds) + if (!dvb->nfeeds) { err = em28xx_stop_streaming(dvb); + if (dev->dev_next) + mutex_lock(&dev->dev_next->lock); + else + mutex_lock(&dev->lock); + + *dev->usage_count = *dev->usage_count - 1; + + if (dev->dev_next) + mutex_unlock(&dev->dev_next->lock); + else + mutex_unlock(&dev->lock); + } + + mutex_unlock(&dvb->lock); return err; } diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h index 953caac..d245c4f 100644 --- a/drivers/media/usb/em28xx/em28xx.h +++ b/drivers/media/usb/em28xx/em28xx.h @@ -775,6 +775,7 @@ struct em28xx { struct em28xx *dev_next; int ts; + unsigned int *usage_count; }; #define kref_to_dev(d) container_of(d, struct em28xx, ref)
Addresses the following, which introduced a regression itself: Commit 509f89652f83 ("media: em28xx: fix a regression with HVR-950") The regression fix breaks dual transport stream support. When a tuner starts streaming it sets alt mode on the USB interface. The problem is both tuners share the same USB interface, so when the second tuner becomes active and sets alt mode on the interface it kills streaming on the other port. It was suggested to add a refcounter and only set alt mode if no tuners are currently active on the interface. This requires sharing some state amongst both tuner devices, with appropriate locking. What I've done here is the following: - Add a usage_count pointer to struct em28xx - Share usage_count between both em28xx devices - Only set alt mode if usage_count is zero - Increment usage_count when each tuner becomes active - Decrement usage_count when a tuner becomes idle With usage_count in the main em28xx struct, locking is handled as follows: - if a secondary tuner exists, lock dev->dev_next->lock - if no secondary tuner exists, lock dev->lock By using the above scheme a single tuner device, will lock itself, the first tuner in a dual tuner device will lock the second tuner, and the second tuner in a dual tuner device will lock itself aka the second tuner instance. Signed-off-by: Brad Love <brad@nextdimension.cc> --- drivers/media/usb/em28xx/em28xx-cards.c | 6 ++++- drivers/media/usb/em28xx/em28xx-dvb.c | 47 +++++++++++++++++++++++++++++++-- drivers/media/usb/em28xx/em28xx.h | 1 + 3 files changed, 51 insertions(+), 3 deletions(-)