Re: [PATCH v2 1/2] Staging: comedi: convert while loops to timeouts in s626.c
>On Wednesday, March 5, 2014 6:10 AM, Ian Abbott wrote: >>On 2014-03-04 08:43, Chase Southwood wrote: >>This patch changes a handful of while loops to timeouts to prevent >>infinite looping on hardware failure. A couple such loops are in a >>function (s626_debi_transfer()) which is called from critical sections, >>so comedi_timeout() is unusable for them, and an iterative timeout is >>used instead. For the while loops in a context where comedi_timeout() is >>allowed, a new callback function, s626_send_dac_eoc(), has been defined >>to evaluate the conditions that the while loops are testing. The new >>callback employs a switch statement based on the register offset values >>that the status is to be checked from, as this information is enough to >>recreate the entire readl() call, and is passed in the 'context' >>parameter. A couple of additional macros have been defined where this >>method is not completely sufficient. The proper comedi_timeout() calls >>are then used. >> >>Signed-off-by: Chase Southwood >>--- >>Ian, my idea for the callback function here is a bit iffy, I'd love to >>hear your feedback. I've created the ret variable because patch 2/2 >>propogates all errors upwards. >> >>2: Now using comedi_timeout() where allowable. [snip] >Rather than switching on a value that is either a register offset or >some special value, it might be better to use an enum to define the >context values you are switching on, e.g.: > >enum { > s626_send_dac_wait_not_mc1_a2out, > s626_send_dac_wait_ssr_af2_out, > s626_send_dac_wait_fb_buffer2_msb_00, > s626_send_dac_wait_fb_buffer2_msb_ff >}; > >Alternatively, different callback functions could be used for each case. Ian, Ah yes...this is much smarter than what I was trying before. I've made up a patch using this enum, and tomorrow I will get PATCH 2/2 rebased and get a v2 of this patchset sent out. Thanks, Chase >One slight oddity about the original code that waits for the MSB of FB >BUFFER2 to go from 0x00 to 0xFF is that it actually waits for it to >become non-zero. I'm presuming the intermediate values between 0x00 and >0xFF are never seen. (There is a non-Comedi, GPL'ed, Linux driver >available from Sensoray that makes the same assumption, but I think the >Comedi driver was derived from it.) > [snip] >-- >-=( Ian Abbott @ MEV Ltd. E-mail: )=- >-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=- >-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] Staging: comedi: convert while loops to timeouts in s626.c
On Wednesday, March 5, 2014 6:10 AM, Ian Abbott abbo...@mev.co.uk wrote: On 2014-03-04 08:43, Chase Southwood wrote: This patch changes a handful of while loops to timeouts to prevent infinite looping on hardware failure. A couple such loops are in a function (s626_debi_transfer()) which is called from critical sections, so comedi_timeout() is unusable for them, and an iterative timeout is used instead. For the while loops in a context where comedi_timeout() is allowed, a new callback function, s626_send_dac_eoc(), has been defined to evaluate the conditions that the while loops are testing. The new callback employs a switch statement based on the register offset values that the status is to be checked from, as this information is enough to recreate the entire readl() call, and is passed in the 'context' parameter. A couple of additional macros have been defined where this method is not completely sufficient. The proper comedi_timeout() calls are then used. Signed-off-by: Chase Southwood chase.southw...@yahoo.com --- Ian, my idea for the callback function here is a bit iffy, I'd love to hear your feedback. I've created the ret variable because patch 2/2 propogates all errors upwards. 2: Now using comedi_timeout() where allowable. [snip] Rather than switching on a value that is either a register offset or some special value, it might be better to use an enum to define the context values you are switching on, e.g.: enum { s626_send_dac_wait_not_mc1_a2out, s626_send_dac_wait_ssr_af2_out, s626_send_dac_wait_fb_buffer2_msb_00, s626_send_dac_wait_fb_buffer2_msb_ff }; Alternatively, different callback functions could be used for each case. Ian, Ah yes...this is much smarter than what I was trying before. I've made up a patch using this enum, and tomorrow I will get PATCH 2/2 rebased and get a v2 of this patchset sent out. Thanks, Chase One slight oddity about the original code that waits for the MSB of FB BUFFER2 to go from 0x00 to 0xFF is that it actually waits for it to become non-zero. I'm presuming the intermediate values between 0x00 and 0xFF are never seen. (There is a non-Comedi, GPL'ed, Linux driver available from Sensoray that makes the same assumption, but I think the Comedi driver was derived from it.) [snip] -- -=( Ian Abbott @ MEV Ltd. E-mail: abbo...@mev.co.uk )=- -=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=- -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] Staging: comedi: convert while loops to timeouts in s626.c
On 2014-03-04 08:43, Chase Southwood wrote: This patch changes a handful of while loops to timeouts to prevent infinite looping on hardware failure. A couple such loops are in a function (s626_debi_transfer()) which is called from critical sections, so comedi_timeout() is unusable for them, and an iterative timeout is used instead. For the while loops in a context where comedi_timeout() is allowed, a new callback function, s626_send_dac_eoc(), has been defined to evaluate the conditions that the while loops are testing. The new callback employs a switch statement based on the register offset values that the status is to be checked from, as this information is enough to recreate the entire readl() call, and is passed in the 'context' parameter. A couple of additional macros have been defined where this method is not completely sufficient. The proper comedi_timeout() calls are then used. Signed-off-by: Chase Southwood --- Ian, my idea for the callback function here is a bit iffy, I'd love to hear your feedback. I've created the ret variable because patch 2/2 propogates all errors upwards. 2: Now using comedi_timeout() where allowable. drivers/staging/comedi/drivers/s626.c | 81 +-- 1 file changed, 69 insertions(+), 12 deletions(-) diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c index 5ba4b4a..5a7e1c0 100644 --- a/drivers/staging/comedi/drivers/s626.c +++ b/drivers/staging/comedi/drivers/s626.c @@ -209,6 +209,8 @@ static const struct comedi_lrange s626_range_table = { static void s626_debi_transfer(struct comedi_device *dev) { struct s626_private *devpriv = dev->private; + static const int timeout = 1; + int i; /* Initiate upload of shadow RAM to DEBI control register */ s626_mc_enable(dev, S626_MC2_UPLD_DEBI, S626_P_MC2); @@ -217,12 +219,22 @@ static void s626_debi_transfer(struct comedi_device *dev) * Wait for completion of upload from shadow RAM to * DEBI control register. */ - while (!s626_mc_test(dev, S626_MC2_UPLD_DEBI, S626_P_MC2)) - ; + for (i = 0; i < timeout; i++) { + if (s626_mc_test(dev, S626_MC2_UPLD_DEBI, S626_P_MC2)) + break; + udelay(1); + } + if (i == timeout) + comedi_error(dev, "Timeout while uploading to DEBI control register."); /* Wait until DEBI transfer is done */ - while (readl(devpriv->mmio + S626_P_PSR) & S626_PSR_DEBI_S) - ; + for (i = 0; i < timeout; i++) { + if (!(readl(devpriv->mmio + S626_P_PSR) & S626_PSR_DEBI_S)) + break; + udelay(1); + } + if (i == timeout) + comedi_error(dev, "DEBI transfer timeout."); } /* @@ -351,6 +363,44 @@ static const uint8_t s626_trimadrs[] = { 0x40, 0x41, 0x42, 0x50, 0x51, 0x52, 0x53, 0x60, 0x61, 0x62, 0x63 }; +#define S626_P_FB_BUFFER2_MSB_00 0 +#define S626_P_FB_BUFFER2_MSB_FF 1 + +static int s626_send_dac_eoc(struct comedi_device *dev, + struct comedi_subdevice *s, + struct comedi_insn *insn, + unsigned long context) +{ + struct s626_private *devpriv = dev->private; + unsigned int status; + + switch (context) { + case S626_P_MC1: + status = readl(devpriv->mmio + S626_P_MC1); + if (!(status & S626_MC1_A2OUT)) + return 0; + break; + case S626_P_SSR: + status = readl(devpriv->mmio + S626_P_SSR); + if (status & S626_SSR_AF2_OUT) + return 0; + break; + case S626_P_FB_BUFFER2_MSB_00: + status = readl(devpriv->mmio + S626_P_FB_BUFFER2); + if (!(status & 0xff00)) + return 0; + break; + case S626_P_FB_BUFFER2_MSB_FF: + status = readl(devpriv->mmio + S626_P_FB_BUFFER2); + if (status & 0xff00) + return 0; + break; + default: + return -EINVAL; + } + return -EBUSY; +} + Rather than switching on a value that is either a register offset or some special value, it might be better to use an enum to define the context values you are switching on, e.g.: enum { s626_send_dac_wait_not_mc1_a2out, s626_send_dac_wait_ssr_af2_out, s626_send_dac_wait_fb_buffer2_msb_00, s626_send_dac_wait_fb_buffer2_msb_ff }; Alternatively, different callback functions could be used for each case. One slight oddity about the original code that waits for the MSB of FB BUFFER2 to go from 0x00 to 0xFF is that it actually waits for it to become non-zero. I'm
Re: [PATCH v2 1/2] Staging: comedi: convert while loops to timeouts in s626.c
On 2014-03-04 08:43, Chase Southwood wrote: This patch changes a handful of while loops to timeouts to prevent infinite looping on hardware failure. A couple such loops are in a function (s626_debi_transfer()) which is called from critical sections, so comedi_timeout() is unusable for them, and an iterative timeout is used instead. For the while loops in a context where comedi_timeout() is allowed, a new callback function, s626_send_dac_eoc(), has been defined to evaluate the conditions that the while loops are testing. The new callback employs a switch statement based on the register offset values that the status is to be checked from, as this information is enough to recreate the entire readl() call, and is passed in the 'context' parameter. A couple of additional macros have been defined where this method is not completely sufficient. The proper comedi_timeout() calls are then used. Signed-off-by: Chase Southwood chase.southw...@yahoo.com --- Ian, my idea for the callback function here is a bit iffy, I'd love to hear your feedback. I've created the ret variable because patch 2/2 propogates all errors upwards. 2: Now using comedi_timeout() where allowable. drivers/staging/comedi/drivers/s626.c | 81 +-- 1 file changed, 69 insertions(+), 12 deletions(-) diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c index 5ba4b4a..5a7e1c0 100644 --- a/drivers/staging/comedi/drivers/s626.c +++ b/drivers/staging/comedi/drivers/s626.c @@ -209,6 +209,8 @@ static const struct comedi_lrange s626_range_table = { static void s626_debi_transfer(struct comedi_device *dev) { struct s626_private *devpriv = dev-private; + static const int timeout = 1; + int i; /* Initiate upload of shadow RAM to DEBI control register */ s626_mc_enable(dev, S626_MC2_UPLD_DEBI, S626_P_MC2); @@ -217,12 +219,22 @@ static void s626_debi_transfer(struct comedi_device *dev) * Wait for completion of upload from shadow RAM to * DEBI control register. */ - while (!s626_mc_test(dev, S626_MC2_UPLD_DEBI, S626_P_MC2)) - ; + for (i = 0; i timeout; i++) { + if (s626_mc_test(dev, S626_MC2_UPLD_DEBI, S626_P_MC2)) + break; + udelay(1); + } + if (i == timeout) + comedi_error(dev, Timeout while uploading to DEBI control register.); /* Wait until DEBI transfer is done */ - while (readl(devpriv-mmio + S626_P_PSR) S626_PSR_DEBI_S) - ; + for (i = 0; i timeout; i++) { + if (!(readl(devpriv-mmio + S626_P_PSR) S626_PSR_DEBI_S)) + break; + udelay(1); + } + if (i == timeout) + comedi_error(dev, DEBI transfer timeout.); } /* @@ -351,6 +363,44 @@ static const uint8_t s626_trimadrs[] = { 0x40, 0x41, 0x42, 0x50, 0x51, 0x52, 0x53, 0x60, 0x61, 0x62, 0x63 }; +#define S626_P_FB_BUFFER2_MSB_00 0 +#define S626_P_FB_BUFFER2_MSB_FF 1 + +static int s626_send_dac_eoc(struct comedi_device *dev, + struct comedi_subdevice *s, + struct comedi_insn *insn, + unsigned long context) +{ + struct s626_private *devpriv = dev-private; + unsigned int status; + + switch (context) { + case S626_P_MC1: + status = readl(devpriv-mmio + S626_P_MC1); + if (!(status S626_MC1_A2OUT)) + return 0; + break; + case S626_P_SSR: + status = readl(devpriv-mmio + S626_P_SSR); + if (status S626_SSR_AF2_OUT) + return 0; + break; + case S626_P_FB_BUFFER2_MSB_00: + status = readl(devpriv-mmio + S626_P_FB_BUFFER2); + if (!(status 0xff00)) + return 0; + break; + case S626_P_FB_BUFFER2_MSB_FF: + status = readl(devpriv-mmio + S626_P_FB_BUFFER2); + if (status 0xff00) + return 0; + break; + default: + return -EINVAL; + } + return -EBUSY; +} + Rather than switching on a value that is either a register offset or some special value, it might be better to use an enum to define the context values you are switching on, e.g.: enum { s626_send_dac_wait_not_mc1_a2out, s626_send_dac_wait_ssr_af2_out, s626_send_dac_wait_fb_buffer2_msb_00, s626_send_dac_wait_fb_buffer2_msb_ff }; Alternatively, different callback functions could be used for each case. One slight oddity about the original code that waits for the MSB of FB BUFFER2 to go from 0x00 to 0xFF is that it actually waits for it to become non-zero.
[PATCH v2 1/2] Staging: comedi: convert while loops to timeouts in s626.c
This patch changes a handful of while loops to timeouts to prevent infinite looping on hardware failure. A couple such loops are in a function (s626_debi_transfer()) which is called from critical sections, so comedi_timeout() is unusable for them, and an iterative timeout is used instead. For the while loops in a context where comedi_timeout() is allowed, a new callback function, s626_send_dac_eoc(), has been defined to evaluate the conditions that the while loops are testing. The new callback employs a switch statement based on the register offset values that the status is to be checked from, as this information is enough to recreate the entire readl() call, and is passed in the 'context' parameter. A couple of additional macros have been defined where this method is not completely sufficient. The proper comedi_timeout() calls are then used. Signed-off-by: Chase Southwood --- Ian, my idea for the callback function here is a bit iffy, I'd love to hear your feedback. I've created the ret variable because patch 2/2 propogates all errors upwards. 2: Now using comedi_timeout() where allowable. drivers/staging/comedi/drivers/s626.c | 81 +-- 1 file changed, 69 insertions(+), 12 deletions(-) diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c index 5ba4b4a..5a7e1c0 100644 --- a/drivers/staging/comedi/drivers/s626.c +++ b/drivers/staging/comedi/drivers/s626.c @@ -209,6 +209,8 @@ static const struct comedi_lrange s626_range_table = { static void s626_debi_transfer(struct comedi_device *dev) { struct s626_private *devpriv = dev->private; + static const int timeout = 1; + int i; /* Initiate upload of shadow RAM to DEBI control register */ s626_mc_enable(dev, S626_MC2_UPLD_DEBI, S626_P_MC2); @@ -217,12 +219,22 @@ static void s626_debi_transfer(struct comedi_device *dev) * Wait for completion of upload from shadow RAM to * DEBI control register. */ - while (!s626_mc_test(dev, S626_MC2_UPLD_DEBI, S626_P_MC2)) - ; + for (i = 0; i < timeout; i++) { + if (s626_mc_test(dev, S626_MC2_UPLD_DEBI, S626_P_MC2)) + break; + udelay(1); + } + if (i == timeout) + comedi_error(dev, "Timeout while uploading to DEBI control register."); /* Wait until DEBI transfer is done */ - while (readl(devpriv->mmio + S626_P_PSR) & S626_PSR_DEBI_S) - ; + for (i = 0; i < timeout; i++) { + if (!(readl(devpriv->mmio + S626_P_PSR) & S626_PSR_DEBI_S)) + break; + udelay(1); + } + if (i == timeout) + comedi_error(dev, "DEBI transfer timeout."); } /* @@ -351,6 +363,44 @@ static const uint8_t s626_trimadrs[] = { 0x40, 0x41, 0x42, 0x50, 0x51, 0x52, 0x53, 0x60, 0x61, 0x62, 0x63 }; +#define S626_P_FB_BUFFER2_MSB_00 0 +#define S626_P_FB_BUFFER2_MSB_FF 1 + +static int s626_send_dac_eoc(struct comedi_device *dev, + struct comedi_subdevice *s, + struct comedi_insn *insn, + unsigned long context) +{ + struct s626_private *devpriv = dev->private; + unsigned int status; + + switch (context) { + case S626_P_MC1: + status = readl(devpriv->mmio + S626_P_MC1); + if (!(status & S626_MC1_A2OUT)) + return 0; + break; + case S626_P_SSR: + status = readl(devpriv->mmio + S626_P_SSR); + if (status & S626_SSR_AF2_OUT) + return 0; + break; + case S626_P_FB_BUFFER2_MSB_00: + status = readl(devpriv->mmio + S626_P_FB_BUFFER2); + if (!(status & 0xff00)) + return 0; + break; + case S626_P_FB_BUFFER2_MSB_FF: + status = readl(devpriv->mmio + S626_P_FB_BUFFER2); + if (status & 0xff00) + return 0; + break; + default: + return -EINVAL; + } + return -EBUSY; +} + /* * Private helper function: Transmit serial data to DAC via Audio * channel 2. Assumes: (1) TSL2 slot records initialized, and (2) @@ -359,6 +409,7 @@ static const uint8_t s626_trimadrs[] = { static void s626_send_dac(struct comedi_device *dev, uint32_t val) { struct s626_private *devpriv = dev->private; + int ret; /* START THE SERIAL CLOCK RUNNING - */ @@ -404,8 +455,9 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val) * Done by polling the DMAC enable flag; this flag is automatically * cleared when the transfer has finished. */ - while
[PATCH v2 1/2] Staging: comedi: convert while loops to timeouts in s626.c
This patch changes a handful of while loops to timeouts to prevent infinite looping on hardware failure. A couple such loops are in a function (s626_debi_transfer()) which is called from critical sections, so comedi_timeout() is unusable for them, and an iterative timeout is used instead. For the while loops in a context where comedi_timeout() is allowed, a new callback function, s626_send_dac_eoc(), has been defined to evaluate the conditions that the while loops are testing. The new callback employs a switch statement based on the register offset values that the status is to be checked from, as this information is enough to recreate the entire readl() call, and is passed in the 'context' parameter. A couple of additional macros have been defined where this method is not completely sufficient. The proper comedi_timeout() calls are then used. Signed-off-by: Chase Southwood chase.southw...@yahoo.com --- Ian, my idea for the callback function here is a bit iffy, I'd love to hear your feedback. I've created the ret variable because patch 2/2 propogates all errors upwards. 2: Now using comedi_timeout() where allowable. drivers/staging/comedi/drivers/s626.c | 81 +-- 1 file changed, 69 insertions(+), 12 deletions(-) diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c index 5ba4b4a..5a7e1c0 100644 --- a/drivers/staging/comedi/drivers/s626.c +++ b/drivers/staging/comedi/drivers/s626.c @@ -209,6 +209,8 @@ static const struct comedi_lrange s626_range_table = { static void s626_debi_transfer(struct comedi_device *dev) { struct s626_private *devpriv = dev-private; + static const int timeout = 1; + int i; /* Initiate upload of shadow RAM to DEBI control register */ s626_mc_enable(dev, S626_MC2_UPLD_DEBI, S626_P_MC2); @@ -217,12 +219,22 @@ static void s626_debi_transfer(struct comedi_device *dev) * Wait for completion of upload from shadow RAM to * DEBI control register. */ - while (!s626_mc_test(dev, S626_MC2_UPLD_DEBI, S626_P_MC2)) - ; + for (i = 0; i timeout; i++) { + if (s626_mc_test(dev, S626_MC2_UPLD_DEBI, S626_P_MC2)) + break; + udelay(1); + } + if (i == timeout) + comedi_error(dev, Timeout while uploading to DEBI control register.); /* Wait until DEBI transfer is done */ - while (readl(devpriv-mmio + S626_P_PSR) S626_PSR_DEBI_S) - ; + for (i = 0; i timeout; i++) { + if (!(readl(devpriv-mmio + S626_P_PSR) S626_PSR_DEBI_S)) + break; + udelay(1); + } + if (i == timeout) + comedi_error(dev, DEBI transfer timeout.); } /* @@ -351,6 +363,44 @@ static const uint8_t s626_trimadrs[] = { 0x40, 0x41, 0x42, 0x50, 0x51, 0x52, 0x53, 0x60, 0x61, 0x62, 0x63 }; +#define S626_P_FB_BUFFER2_MSB_00 0 +#define S626_P_FB_BUFFER2_MSB_FF 1 + +static int s626_send_dac_eoc(struct comedi_device *dev, + struct comedi_subdevice *s, + struct comedi_insn *insn, + unsigned long context) +{ + struct s626_private *devpriv = dev-private; + unsigned int status; + + switch (context) { + case S626_P_MC1: + status = readl(devpriv-mmio + S626_P_MC1); + if (!(status S626_MC1_A2OUT)) + return 0; + break; + case S626_P_SSR: + status = readl(devpriv-mmio + S626_P_SSR); + if (status S626_SSR_AF2_OUT) + return 0; + break; + case S626_P_FB_BUFFER2_MSB_00: + status = readl(devpriv-mmio + S626_P_FB_BUFFER2); + if (!(status 0xff00)) + return 0; + break; + case S626_P_FB_BUFFER2_MSB_FF: + status = readl(devpriv-mmio + S626_P_FB_BUFFER2); + if (status 0xff00) + return 0; + break; + default: + return -EINVAL; + } + return -EBUSY; +} + /* * Private helper function: Transmit serial data to DAC via Audio * channel 2. Assumes: (1) TSL2 slot records initialized, and (2) @@ -359,6 +409,7 @@ static const uint8_t s626_trimadrs[] = { static void s626_send_dac(struct comedi_device *dev, uint32_t val) { struct s626_private *devpriv = dev-private; + int ret; /* START THE SERIAL CLOCK RUNNING - */ @@ -404,8 +455,9 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val) * Done by polling the DMAC enable flag; this flag is automatically * cleared when the transfer has finished. */ - while