diff mbox

[2/2] Added timers for dvb_ca_en50221_write_data

Message ID 1505611642-6552-3-git-send-email-jasmin@anw.at (mailing list archive)
State New, archived
Headers show

Commit Message

Jasmin J. Sept. 17, 2017, 1:27 a.m. UTC 
From: Jasmin Jessich <jasmin@anw.at>

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 <jasmin@anw.at>
---
 drivers/media/dvb-core/dvb_ca_en50221.c | 132 +++++++++++++++++++++++++++++++-
 1 file changed, 131 insertions(+), 1 deletion(-)

Comments

Mauro Carvalho Chehab Dec. 14, 2017, 12:03 p.m. UTC | #1 
Em Sun, 17 Sep 2017 03:27:22 +0200
"Jasmin J." <jasmin@anw.at> escreveu:

> From: Jasmin Jessich <jasmin@anw.at>
> 
> 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".

The patch series looks OK. However, I'm not seeing any documentation
patches explaining the new sysfs interface. I would be expecting an
update at linux DVB uAPI. Also, for all sysfs stuff, you need to add
it to Documentation/ABI/ using the format explained at its README.

Once you add it, I'll gladly apply it. Please add Ralph's ack on the
existing patches on your next submission.

Just a minor nitpick: on all patches, please prepend with "media: "
and the name of the part of the media subsystem that you're patching.

In this case, a good subject would be:

	media: dvb-core: add timers for dvb_ca_en50221_write_data

Regards,
Mauro

> 
> Signed-off-by: Jasmin Jessich <jasmin@anw.at>
> ---
>  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 95b3723..50c4e45 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 */
>  
> @@ -868,10 +896,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++) {
> @@ -879,6 +910,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) */
> @@ -1832,6 +1864,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,
> +			       struct device_attribute *attr, char *buf)
> +{
> +	struct dvb_device *dvbdev = dev_get_drvdata(device);
> +	struct dvb_ca_private *ca = dvbdev->priv;
> +	int tim_idx = dvb_ca_tim_idx(ca, attr->attr.name);
> +
> +	if (tim_idx < 0)
> +		return -ENXIO;
> +
> +	return sprintf(buf, "%ld\n", ca->timers[tim_idx].min);
> +}
> +
> +static ssize_t dvb_ca_tim_store(struct device *device,
> +				struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	struct dvb_device *dvbdev = dev_get_drvdata(device);
> +	struct dvb_ca_private *ca = dvbdev->priv;
> +	int tim_idx = dvb_ca_tim_idx(ca, attr->attr.name);
> +	unsigned long min, max;
> +
> +	if (tim_idx < 0)
> +		return -ENXIO;
> +
> +	if (sscanf(buf, "%lu\n", &min) != 1)
> +		return -EINVAL;
> +
> +	/* value is in us; 100ms is a good maximum */
> +	if (min > (100 * USEC_PER_MSEC))
> +		return -EINVAL;
> +
> +	/* +10% (rounded up) */
> +	max = (min * 11 + 5) / 10;
> +	ca->timers[tim_idx].min = min;
> +	ca->timers[tim_idx].max = max;
> +
> +	return count;
> +}
> +
> +/* attribute definition with string pointer (see include/linux/sysfs.h) */
> +#define DVB_CA_ATTR(_name, _mode, _show, _store) {	\
> +	.attr = {.name = _name, .mode = _mode },	\
> +	.show	= _show,				\
> +	.store	= _store,				\
> +}
> +
> +#define DVB_CA_ATTR_TIM(_tim_idx)					\
> +	DVB_CA_ATTR(dvb_ca_tim_names[_tim_idx], 0664, dvb_ca_tim_show,	\
> +		    dvb_ca_tim_store)
> +
> +static const struct device_attribute dvb_ca_attrs[DVB_CA_TIM_MAX] = {
> +	DVB_CA_ATTR_TIM(DVB_CA_TIM_WR_HIGH)
> +,	DVB_CA_ATTR_TIM(DVB_CA_TIM_WR_LOW)
> +,	DVB_CA_ATTR_TIM(DVB_CA_TIM_WR_DATA)
> +};
> +
> +static int dvb_ca_device_attrs_add(struct dvb_ca_private *ca)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(dvb_ca_attrs); i++)
> +		if (device_create_file(ca->dvbdev->dev, &dvb_ca_attrs[i]))
> +			goto fail;
> +	return 0;
> +fail:
> +	return -1;
> +}
> +
> +static void ddb_device_attrs_del(struct dvb_ca_private *ca)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(dvb_ca_attrs); i++)
> +		device_remove_file(ca->dvbdev->dev, &dvb_ca_attrs[i]);
> +}
> +
> +/* ************************************************************************** */
>  /* Initialisation/shutdown functions */
>  
>  /**
> @@ -1901,6 +2024,10 @@ int dvb_ca_en50221_init(struct dvb_adapter *dvb_adapter,
>  		ret = -EINTR;
>  		goto unregister_device;
>  	}
> +
> +	if (dvb_ca_device_attrs_add(ca))
> +		goto unregister_device;
> +
>  	mb();
>  
>  	/* create a kthread for monitoring this CA device */
> @@ -1910,10 +2037,12 @@ int dvb_ca_en50221_init(struct dvb_adapter *dvb_adapter,
>  		ret = PTR_ERR(ca->thread);
>  		pr_err("dvb_ca_init: failed to start kernel_thread (%d)\n",
>  		       ret);
> -		goto unregister_device;
> +		goto delete_attrs;
>  	}
>  	return 0;
>  
> +delete_attrs:
> +	ddb_device_attrs_del(ca);
>  unregister_device:
>  	dvb_unregister_device(ca->dvbdev);
>  free_slot_info:
> @@ -1945,6 +2074,7 @@ void dvb_ca_en50221_release(struct dvb_ca_en50221 *pubca)
>  	for (i = 0; i < ca->slot_count; i++)
>  		dvb_ca_en50221_slot_shutdown(ca, i);
>  
> +	ddb_device_attrs_del(ca);
>  	dvb_remove_device(ca->dvbdev);
>  	dvb_ca_private_put(ca);
>  	pubca->private = NULL;



Thanks,
Mauro
diff mbox

Patch

diff --git a/drivers/media/dvb-core/dvb_ca_en50221.c b/drivers/media/dvb-core/dvb_ca_en50221.c
index 95b3723..50c4e45 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 */
 
@@ -868,10 +896,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++) {
@@ -879,6 +910,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) */
@@ -1832,6 +1864,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,
+			       struct device_attribute *attr, char *buf)
+{
+	struct dvb_device *dvbdev = dev_get_drvdata(device);
+	struct dvb_ca_private *ca = dvbdev->priv;
+	int tim_idx = dvb_ca_tim_idx(ca, attr->attr.name);
+
+	if (tim_idx < 0)
+		return -ENXIO;
+
+	return sprintf(buf, "%ld\n", ca->timers[tim_idx].min);
+}
+
+static ssize_t dvb_ca_tim_store(struct device *device,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct dvb_device *dvbdev = dev_get_drvdata(device);
+	struct dvb_ca_private *ca = dvbdev->priv;
+	int tim_idx = dvb_ca_tim_idx(ca, attr->attr.name);
+	unsigned long min, max;
+
+	if (tim_idx < 0)
+		return -ENXIO;
+
+	if (sscanf(buf, "%lu\n", &min) != 1)
+		return -EINVAL;
+
+	/* value is in us; 100ms is a good maximum */
+	if (min > (100 * USEC_PER_MSEC))
+		return -EINVAL;
+
+	/* +10% (rounded up) */
+	max = (min * 11 + 5) / 10;
+	ca->timers[tim_idx].min = min;
+	ca->timers[tim_idx].max = max;
+
+	return count;
+}
+
+/* attribute definition with string pointer (see include/linux/sysfs.h) */
+#define DVB_CA_ATTR(_name, _mode, _show, _store) {	\
+	.attr = {.name = _name, .mode = _mode },	\
+	.show	= _show,				\
+	.store	= _store,				\
+}
+
+#define DVB_CA_ATTR_TIM(_tim_idx)					\
+	DVB_CA_ATTR(dvb_ca_tim_names[_tim_idx], 0664, dvb_ca_tim_show,	\
+		    dvb_ca_tim_store)
+
+static const struct device_attribute dvb_ca_attrs[DVB_CA_TIM_MAX] = {
+	DVB_CA_ATTR_TIM(DVB_CA_TIM_WR_HIGH)
+,	DVB_CA_ATTR_TIM(DVB_CA_TIM_WR_LOW)
+,	DVB_CA_ATTR_TIM(DVB_CA_TIM_WR_DATA)
+};
+
+static int dvb_ca_device_attrs_add(struct dvb_ca_private *ca)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(dvb_ca_attrs); i++)
+		if (device_create_file(ca->dvbdev->dev, &dvb_ca_attrs[i]))
+			goto fail;
+	return 0;
+fail:
+	return -1;
+}
+
+static void ddb_device_attrs_del(struct dvb_ca_private *ca)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(dvb_ca_attrs); i++)
+		device_remove_file(ca->dvbdev->dev, &dvb_ca_attrs[i]);
+}
+
+/* ************************************************************************** */
 /* Initialisation/shutdown functions */
 
 /**
@@ -1901,6 +2024,10 @@  int dvb_ca_en50221_init(struct dvb_adapter *dvb_adapter,
 		ret = -EINTR;
 		goto unregister_device;
 	}
+
+	if (dvb_ca_device_attrs_add(ca))
+		goto unregister_device;
+
 	mb();
 
 	/* create a kthread for monitoring this CA device */
@@ -1910,10 +2037,12 @@  int dvb_ca_en50221_init(struct dvb_adapter *dvb_adapter,
 		ret = PTR_ERR(ca->thread);
 		pr_err("dvb_ca_init: failed to start kernel_thread (%d)\n",
 		       ret);
-		goto unregister_device;
+		goto delete_attrs;
 	}
 	return 0;
 
+delete_attrs:
+	ddb_device_attrs_del(ca);
 unregister_device:
 	dvb_unregister_device(ca->dvbdev);
 free_slot_info:
@@ -1945,6 +2074,7 @@  void dvb_ca_en50221_release(struct dvb_ca_en50221 *pubca)
 	for (i = 0; i < ca->slot_count; i++)
 		dvb_ca_en50221_slot_shutdown(ca, i);
 
+	ddb_device_attrs_del(ca);
 	dvb_remove_device(ca->dvbdev);
 	dvb_ca_private_put(ca);
 	pubca->private = NULL;