Re: [PATCH V2 2/3] media: dvb-core: Added timers for dvb_ca_en50221_write_data

[Mauro Carvalho Chehab]

Em Wed, 14 Feb 2018 00:29:43 +0100
"Jasmin J."  escreveu:

> Hi!
> 
> Please hold on in merging this series, because I have to investigate a hint
> I got related to the buffer size handshake of the protocol driver:
>   https://www.linuxtv.org/pipermail/linux-dvb/2007-July/019116.html

Ok.

I'll mark this series as "Changes requested".

Please resubmit it once you have a new patch series.

> 
> BR,
>Jasmin
> 
> 
> On 12/21/2017 02:22 PM, Jasmin J. wrote:
> > From: Jasmin Jessich 
> > 
> > Some (older) CAMs are really slow in accepting data. The CI interface
> > specification doesn't define a handshake for accepted data. Thus, the
> > en50221 protocol driver can't control if a data byte has been correctly
> > written to the CAM.
> > 
> > The current implementation writes the length and the data quick after
> > each other. Thus, the slow CAMs may generate a WR error, which leads to
> > the known error logging
> >"CAM tried to send a buffer larger than the ecount size".
> > 
> > To solve this issue the en50221 protocol driver needs to wait some CAM
> > depending time between the different bytes to be written. Because the
> > time is CAM dependent, an individual value per CAM needs to be set. For
> > that SysFS is used in favor of ioctl's to allow the control of the timer
> > values independent from any user space application.
> > 
> > This patch adds the timers and the SysFS nodes to set/get the timeout
> > values and the timer waiting between the different steps of the CAM write
> > access. A timer value of 0 (default) means "no timeout".
> > 
> > Signed-off-by: Jasmin Jessich 
> > Acked-by: Ralph Metzler 
> > ---
> >  drivers/media/dvb-core/dvb_ca_en50221.c | 132 
> > +++-
> >  1 file changed, 131 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/dvb-core/dvb_ca_en50221.c 
> > b/drivers/media/dvb-core/dvb_ca_en50221.c
> > index a3b2754..9b45d6b 100644
> > --- a/drivers/media/dvb-core/dvb_ca_en50221.c
> > +++ b/drivers/media/dvb-core/dvb_ca_en50221.c
> > @@ -86,6 +86,13 @@ MODULE_PARM_DESC(cam_debug, "enable verbose debug 
> > messages");
> >  #define DVB_CA_SLOTSTATE_WAITFR 6
> >  #define DVB_CA_SLOTSTATE_LINKINIT   7
> >  
> > +enum dvb_ca_timers {
> > +   DVB_CA_TIM_WR_HIGH  /* wait after writing length high */
> > +,  DVB_CA_TIM_WR_LOW   /* wait after writing length low */
> > +,  DVB_CA_TIM_WR_DATA  /* wait between data bytes */
> > +,  DVB_CA_TIM_MAX
> > +};
> > +
> >  /* Information on a CA slot */
> >  struct dvb_ca_slot {
> > /* current state of the CAM */
> > @@ -119,6 +126,11 @@ struct dvb_ca_slot {
> > unsigned long timeout;
> >  };
> >  
> > +struct dvb_ca_timer {
> > +   unsigned long min;
> > +   unsigned long max;
> > +};
> > +
> >  /* Private CA-interface information */
> >  struct dvb_ca_private {
> > struct kref refcount;
> > @@ -161,6 +173,14 @@ struct dvb_ca_private {
> >  
> > /* mutex serializing ioctls */
> > struct mutex ioctl_mutex;
> > +
> > +   struct dvb_ca_timer timers[DVB_CA_TIM_MAX];
> > +};
> > +
> > +static const char dvb_ca_tim_names[DVB_CA_TIM_MAX][15] = {
> > +   "tim_wr_high"
> > +,  "tim_wr_low"
> > +,  "tim_wr_data"
> >  };
> >  
> >  static void dvb_ca_private_free(struct dvb_ca_private *ca)
> > @@ -223,6 +243,14 @@ static char *findstr(char *haystack, int hlen, char 
> > *needle, int nlen)
> > return NULL;
> >  }
> >  
> > +static void dvb_ca_sleep(struct dvb_ca_private *ca, enum dvb_ca_timers tim)
> > +{
> > +   unsigned long min = ca->timers[tim].min;
> > +
> > +   if (min)
> > +   usleep_range(min, ca->timers[tim].max);
> > +}
> > +
> >  /* 
> > ** 
> > */
> >  /* EN50221 physical interface functions */
> >  
> > @@ -869,10 +897,13 @@ static int dvb_ca_en50221_write_data(struct 
> > dvb_ca_private *ca, int slot,
> > bytes_write >> 8);
> > if (status)
> > goto exit;
> > +   dvb_ca_sleep(ca, DVB_CA_TIM_WR_HIGH);
> > +
> > status = ca->pub->write_cam_control(ca->pub, slot, CTRLIF_SIZE_LOW,
> > bytes_write & 0xff);
> > if (status)
> > goto exit;
> > +   dvb_ca_sleep(ca, DVB_CA_TIM_WR_LOW);
> >  
> > /* send the buffer */
> > for (i = 0; i < bytes_write; i++) {
> > @@ -880,6 +911,7 @@ static int dvb_ca_en50221_write_data(struct 
> > dvb_ca_private *ca, int slot,
> > buf[i]);
> > if (status)
> > goto exit;
> > +   dvb_ca_sleep(ca, DVB_CA_TIM_WR_DATA);
> > }
> >  
> > /* check for write error (WE should now be 0) */
> > @@ -1834,6 +1866,97 @@ static const struct dvb_device dvbdev_ca = {
> >  };
> >  
> >  /* 
> > ** 
> > */
> > +/* EN50221 device 

Re: [PATCH V2 2/3] media: dvb-core: Added timers for dvb_ca_en50221_write_data

[Jasmin J.]

Hi!

Please hold on in merging this series, because I have to investigate a hint
I got related to the buffer size handshake of the protocol driver:
  https://www.linuxtv.org/pipermail/linux-dvb/2007-July/019116.html

BR,
   Jasmin


On 12/21/2017 02:22 PM, Jasmin J. wrote:
> From: Jasmin Jessich 
> 
> Some (older) CAMs are really slow in accepting data. The CI interface
> specification doesn't define a handshake for accepted data. Thus, the
> en50221 protocol driver can't control if a data byte has been correctly
> written to the CAM.
> 
> The current implementation writes the length and the data quick after
> each other. Thus, the slow CAMs may generate a WR error, which leads to
> the known error logging
>"CAM tried to send a buffer larger than the ecount size".
> 
> To solve this issue the en50221 protocol driver needs to wait some CAM
> depending time between the different bytes to be written. Because the
> time is CAM dependent, an individual value per CAM needs to be set. For
> that SysFS is used in favor of ioctl's to allow the control of the timer
> values independent from any user space application.
> 
> This patch adds the timers and the SysFS nodes to set/get the timeout
> values and the timer waiting between the different steps of the CAM write
> access. A timer value of 0 (default) means "no timeout".
> 
> Signed-off-by: Jasmin Jessich 
> Acked-by: Ralph Metzler 
> ---
>  drivers/media/dvb-core/dvb_ca_en50221.c | 132 
> +++-
>  1 file changed, 131 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/dvb-core/dvb_ca_en50221.c 
> b/drivers/media/dvb-core/dvb_ca_en50221.c
> index a3b2754..9b45d6b 100644
> --- a/drivers/media/dvb-core/dvb_ca_en50221.c
> +++ b/drivers/media/dvb-core/dvb_ca_en50221.c
> @@ -86,6 +86,13 @@ MODULE_PARM_DESC(cam_debug, "enable verbose debug 
> messages");
>  #define DVB_CA_SLOTSTATE_WAITFR 6
>  #define DVB_CA_SLOTSTATE_LINKINIT   7
>  
> +enum dvb_ca_timers {
> + DVB_CA_TIM_WR_HIGH  /* wait after writing length high */
> +,DVB_CA_TIM_WR_LOW   /* wait after writing length low */
> +,DVB_CA_TIM_WR_DATA  /* wait between data bytes */
> +,DVB_CA_TIM_MAX
> +};
> +
>  /* Information on a CA slot */
>  struct dvb_ca_slot {
>   /* current state of the CAM */
> @@ -119,6 +126,11 @@ struct dvb_ca_slot {
>   unsigned long timeout;
>  };
>  
> +struct dvb_ca_timer {
> + unsigned long min;
> + unsigned long max;
> +};
> +
>  /* Private CA-interface information */
>  struct dvb_ca_private {
>   struct kref refcount;
> @@ -161,6 +173,14 @@ struct dvb_ca_private {
>  
>   /* mutex serializing ioctls */
>   struct mutex ioctl_mutex;
> +
> + struct dvb_ca_timer timers[DVB_CA_TIM_MAX];
> +};
> +
> +static const char dvb_ca_tim_names[DVB_CA_TIM_MAX][15] = {
> + "tim_wr_high"
> +,"tim_wr_low"
> +,"tim_wr_data"
>  };
>  
>  static void dvb_ca_private_free(struct dvb_ca_private *ca)
> @@ -223,6 +243,14 @@ static char *findstr(char *haystack, int hlen, char 
> *needle, int nlen)
>   return NULL;
>  }
>  
> +static void dvb_ca_sleep(struct dvb_ca_private *ca, enum dvb_ca_timers tim)
> +{
> + unsigned long min = ca->timers[tim].min;
> +
> + if (min)
> + usleep_range(min, ca->timers[tim].max);
> +}
> +
>  /* 
> ** */
>  /* EN50221 physical interface functions */
>  
> @@ -869,10 +897,13 @@ static int dvb_ca_en50221_write_data(struct 
> dvb_ca_private *ca, int slot,
>   bytes_write >> 8);
>   if (status)
>   goto exit;
> + dvb_ca_sleep(ca, DVB_CA_TIM_WR_HIGH);
> +
>   status = ca->pub->write_cam_control(ca->pub, slot, CTRLIF_SIZE_LOW,
>   bytes_write & 0xff);
>   if (status)
>   goto exit;
> + dvb_ca_sleep(ca, DVB_CA_TIM_WR_LOW);
>  
>   /* send the buffer */
>   for (i = 0; i < bytes_write; i++) {
> @@ -880,6 +911,7 @@ static int dvb_ca_en50221_write_data(struct 
> dvb_ca_private *ca, int slot,
>   buf[i]);
>   if (status)
>   goto exit;
> + dvb_ca_sleep(ca, DVB_CA_TIM_WR_DATA);
>   }
>  
>   /* check for write error (WE should now be 0) */
> @@ -1834,6 +1866,97 @@ static const struct dvb_device dvbdev_ca = {
>  };
>  
>  /* 
> ** */
> +/* EN50221 device attributes (SysFS) */
> +
> +static int dvb_ca_tim_idx(struct dvb_ca_private *ca, const char *name)
> +{
> + int tim_idx;
> +
> + for (tim_idx = 0; tim_idx < DVB_CA_TIM_MAX; tim_idx++) {
> + if (!strcmp(dvb_ca_tim_names[tim_idx], name))
> + return tim_idx;
> + }
> + return -1;
> +}
> +
> +static ssize_t dvb_ca_tim_show(struct device *device,
> +