diff mbox series

media: gpio-ir-tx: spinlock is not needed to disable interrupts

Message ID 20200813101841.5526-1-sean@mess.org (mailing list archive)
State New, archived
Headers show
Series media: gpio-ir-tx: spinlock is not needed to disable interrupts | expand

Commit Message

Sean Young Aug. 13, 2020, 10:18 a.m. UTC 
During bit-banging the IR on a gpio pin, we cannot be scheduled or have
anything interrupt us, else the generated signal will be incorrect.
Therefore, we need to disable interrupts on the local cpu. This also
disables preemption.

local_irq_disable() does exactly what we need and does not require a
spinlock.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/gpio-ir-tx.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

Comments

kernel test robot Aug. 13, 2020, 12:04 p.m. UTC | #1 
Hi Sean,

I love your patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v5.8 next-20200813]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sean-Young/media-gpio-ir-tx-spinlock-is-not-needed-to-disable-interrupts/20200813-182045
base:   git://linuxtv.org/media_tree.git master
config: h8300-randconfig-r011-20200813 (attached as .config)
compiler: h8300-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=h8300 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/kernel.h:11,
                    from drivers/media/rc/gpio-ir-tx.c:6:
   include/linux/scatterlist.h: In function 'sg_set_buf':
   include/asm-generic/page.h:93:50: warning: ordered comparison of pointer with null pointer [-Wextra]
      93 | #define virt_addr_valid(kaddr) (((void *)(kaddr) >= (void *)PAGE_OFFSET) && \
         |                                                  ^~
   include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
      78 | # define unlikely(x) __builtin_expect(!!(x), 0)
         |                                          ^
   include/linux/scatterlist.h:143:2: note: in expansion of macro 'BUG_ON'
     143 |  BUG_ON(!virt_addr_valid(buf));
         |  ^~~~~~
   include/linux/scatterlist.h:143:10: note: in expansion of macro 'virt_addr_valid'
     143 |  BUG_ON(!virt_addr_valid(buf));
         |          ^~~~~~~~~~~~~~~
   In file included from include/linux/seqlock.h:36,
                    from include/linux/time.h:6,
                    from include/linux/stat.h:19,
                    from include/linux/module.h:13,
                    from drivers/media/rc/gpio-ir-tx.c:7:
   drivers/media/rc/gpio-ir-tx.c: In function 'gpio_ir_tx_probe':
>> drivers/media/rc/gpio-ir-tx.c:174:25: error: 'struct gpio_ir' has no member named 'lock'
     174 |  spin_lock_init(&gpio_ir->lock);
         |                         ^~
   include/linux/spinlock.h:345:17: note: in definition of macro 'spin_lock_init'
     345 |  spinlock_check(_lock);   \
         |                 ^~~~~
>> drivers/media/rc/gpio-ir-tx.c:174:25: error: 'struct gpio_ir' has no member named 'lock'
     174 |  spin_lock_init(&gpio_ir->lock);
         |                         ^~
   include/linux/spinlock.h:346:4: note: in definition of macro 'spin_lock_init'
     346 |  *(_lock) = __SPIN_LOCK_UNLOCKED(_lock); \
         |    ^~~~~

vim +174 drivers/media/rc/gpio-ir-tx.c

24d79ebc6ccec55 Sean Young 2017-07-07  142  
24d79ebc6ccec55 Sean Young 2017-07-07  143  static int gpio_ir_tx_probe(struct platform_device *pdev)
24d79ebc6ccec55 Sean Young 2017-07-07  144  {
24d79ebc6ccec55 Sean Young 2017-07-07  145  	struct gpio_ir *gpio_ir;
24d79ebc6ccec55 Sean Young 2017-07-07  146  	struct rc_dev *rcdev;
24d79ebc6ccec55 Sean Young 2017-07-07  147  	int rc;
24d79ebc6ccec55 Sean Young 2017-07-07  148  
24d79ebc6ccec55 Sean Young 2017-07-07  149  	gpio_ir = devm_kmalloc(&pdev->dev, sizeof(*gpio_ir), GFP_KERNEL);
24d79ebc6ccec55 Sean Young 2017-07-07  150  	if (!gpio_ir)
24d79ebc6ccec55 Sean Young 2017-07-07  151  		return -ENOMEM;
24d79ebc6ccec55 Sean Young 2017-07-07  152  
24d79ebc6ccec55 Sean Young 2017-07-07  153  	rcdev = devm_rc_allocate_device(&pdev->dev, RC_DRIVER_IR_RAW_TX);
24d79ebc6ccec55 Sean Young 2017-07-07  154  	if (!rcdev)
24d79ebc6ccec55 Sean Young 2017-07-07  155  		return -ENOMEM;
24d79ebc6ccec55 Sean Young 2017-07-07  156  
24d79ebc6ccec55 Sean Young 2017-07-07  157  	gpio_ir->gpio = devm_gpiod_get(&pdev->dev, NULL, GPIOD_OUT_LOW);
24d79ebc6ccec55 Sean Young 2017-07-07  158  	if (IS_ERR(gpio_ir->gpio)) {
24d79ebc6ccec55 Sean Young 2017-07-07  159  		if (PTR_ERR(gpio_ir->gpio) != -EPROBE_DEFER)
24d79ebc6ccec55 Sean Young 2017-07-07  160  			dev_err(&pdev->dev, "Failed to get gpio (%ld)\n",
24d79ebc6ccec55 Sean Young 2017-07-07  161  				PTR_ERR(gpio_ir->gpio));
24d79ebc6ccec55 Sean Young 2017-07-07  162  		return PTR_ERR(gpio_ir->gpio);
24d79ebc6ccec55 Sean Young 2017-07-07  163  	}
24d79ebc6ccec55 Sean Young 2017-07-07  164  
24d79ebc6ccec55 Sean Young 2017-07-07  165  	rcdev->priv = gpio_ir;
24d79ebc6ccec55 Sean Young 2017-07-07  166  	rcdev->driver_name = DRIVER_NAME;
24d79ebc6ccec55 Sean Young 2017-07-07  167  	rcdev->device_name = DEVICE_NAME;
24d79ebc6ccec55 Sean Young 2017-07-07  168  	rcdev->tx_ir = gpio_ir_tx;
24d79ebc6ccec55 Sean Young 2017-07-07  169  	rcdev->s_tx_duty_cycle = gpio_ir_tx_set_duty_cycle;
24d79ebc6ccec55 Sean Young 2017-07-07  170  	rcdev->s_tx_carrier = gpio_ir_tx_set_carrier;
24d79ebc6ccec55 Sean Young 2017-07-07  171  
24d79ebc6ccec55 Sean Young 2017-07-07  172  	gpio_ir->carrier = 38000;
24d79ebc6ccec55 Sean Young 2017-07-07  173  	gpio_ir->duty_cycle = 50;
24d79ebc6ccec55 Sean Young 2017-07-07 @174  	spin_lock_init(&gpio_ir->lock);
24d79ebc6ccec55 Sean Young 2017-07-07  175  
24d79ebc6ccec55 Sean Young 2017-07-07  176  	rc = devm_rc_register_device(&pdev->dev, rcdev);
24d79ebc6ccec55 Sean Young 2017-07-07  177  	if (rc < 0)
24d79ebc6ccec55 Sean Young 2017-07-07  178  		dev_err(&pdev->dev, "failed to register rc device\n");
24d79ebc6ccec55 Sean Young 2017-07-07  179  
24d79ebc6ccec55 Sean Young 2017-07-07  180  	return rc;
24d79ebc6ccec55 Sean Young 2017-07-07  181  }
24d79ebc6ccec55 Sean Young 2017-07-07  182  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Sean Young Aug. 13, 2020, 1:24 p.m. UTC | #2 
On Thu, Aug 13, 2020 at 08:04:40PM +0800, kernel test robot wrote:
> Hi Sean,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on linuxtv-media/master]
> [also build test ERROR on v5.8 next-20200813]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Sean-Young/media-gpio-ir-tx-spinlock-is-not-needed-to-disable-interrupts/20200813-182045
> base:   git://linuxtv.org/media_tree.git master
> config: h8300-randconfig-r011-20200813 (attached as .config)
> compiler: h8300-linux-gcc (GCC) 9.3.0
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=h8300 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>    In file included from include/linux/kernel.h:11,
>                     from drivers/media/rc/gpio-ir-tx.c:6:
>    include/linux/scatterlist.h: In function 'sg_set_buf':
>    include/asm-generic/page.h:93:50: warning: ordered comparison of pointer with null pointer [-Wextra]
>       93 | #define virt_addr_valid(kaddr) (((void *)(kaddr) >= (void *)PAGE_OFFSET) && \
>          |                                                  ^~
>    include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
>       78 | # define unlikely(x) __builtin_expect(!!(x), 0)
>          |                                          ^
>    include/linux/scatterlist.h:143:2: note: in expansion of macro 'BUG_ON'
>      143 |  BUG_ON(!virt_addr_valid(buf));
>          |  ^~~~~~
>    include/linux/scatterlist.h:143:10: note: in expansion of macro 'virt_addr_valid'
>      143 |  BUG_ON(!virt_addr_valid(buf));
>          |          ^~~~~~~~~~~~~~~
>    In file included from include/linux/seqlock.h:36,
>                     from include/linux/time.h:6,
>                     from include/linux/stat.h:19,
>                     from include/linux/module.h:13,
>                     from drivers/media/rc/gpio-ir-tx.c:7:
>    drivers/media/rc/gpio-ir-tx.c: In function 'gpio_ir_tx_probe':
> >> drivers/media/rc/gpio-ir-tx.c:174:25: error: 'struct gpio_ir' has no member named 'lock'
>      174 |  spin_lock_init(&gpio_ir->lock);
>          |                         ^~
>    include/linux/spinlock.h:345:17: note: in definition of macro 'spin_lock_init'
>      345 |  spinlock_check(_lock);   \
>          |                 ^~~~~
> >> drivers/media/rc/gpio-ir-tx.c:174:25: error: 'struct gpio_ir' has no member named 'lock'
>      174 |  spin_lock_init(&gpio_ir->lock);
>          |                         ^~
>    include/linux/spinlock.h:346:4: note: in definition of macro 'spin_lock_init'
>      346 |  *(_lock) = __SPIN_LOCK_UNLOCKED(_lock); \
>          |    ^~~~~
> 
> vim +174 drivers/media/rc/gpio-ir-tx.c
> 
> 24d79ebc6ccec55 Sean Young 2017-07-07  142  
> 24d79ebc6ccec55 Sean Young 2017-07-07  143  static int gpio_ir_tx_probe(struct platform_device *pdev)
> 24d79ebc6ccec55 Sean Young 2017-07-07  144  {
> 24d79ebc6ccec55 Sean Young 2017-07-07  145  	struct gpio_ir *gpio_ir;
> 24d79ebc6ccec55 Sean Young 2017-07-07  146  	struct rc_dev *rcdev;
> 24d79ebc6ccec55 Sean Young 2017-07-07  147  	int rc;
> 24d79ebc6ccec55 Sean Young 2017-07-07  148  
> 24d79ebc6ccec55 Sean Young 2017-07-07  149  	gpio_ir = devm_kmalloc(&pdev->dev, sizeof(*gpio_ir), GFP_KERNEL);
> 24d79ebc6ccec55 Sean Young 2017-07-07  150  	if (!gpio_ir)
> 24d79ebc6ccec55 Sean Young 2017-07-07  151  		return -ENOMEM;
> 24d79ebc6ccec55 Sean Young 2017-07-07  152  
> 24d79ebc6ccec55 Sean Young 2017-07-07  153  	rcdev = devm_rc_allocate_device(&pdev->dev, RC_DRIVER_IR_RAW_TX);
> 24d79ebc6ccec55 Sean Young 2017-07-07  154  	if (!rcdev)
> 24d79ebc6ccec55 Sean Young 2017-07-07  155  		return -ENOMEM;
> 24d79ebc6ccec55 Sean Young 2017-07-07  156  
> 24d79ebc6ccec55 Sean Young 2017-07-07  157  	gpio_ir->gpio = devm_gpiod_get(&pdev->dev, NULL, GPIOD_OUT_LOW);
> 24d79ebc6ccec55 Sean Young 2017-07-07  158  	if (IS_ERR(gpio_ir->gpio)) {
> 24d79ebc6ccec55 Sean Young 2017-07-07  159  		if (PTR_ERR(gpio_ir->gpio) != -EPROBE_DEFER)
> 24d79ebc6ccec55 Sean Young 2017-07-07  160  			dev_err(&pdev->dev, "Failed to get gpio (%ld)\n",
> 24d79ebc6ccec55 Sean Young 2017-07-07  161  				PTR_ERR(gpio_ir->gpio));
> 24d79ebc6ccec55 Sean Young 2017-07-07  162  		return PTR_ERR(gpio_ir->gpio);
> 24d79ebc6ccec55 Sean Young 2017-07-07  163  	}
> 24d79ebc6ccec55 Sean Young 2017-07-07  164  
> 24d79ebc6ccec55 Sean Young 2017-07-07  165  	rcdev->priv = gpio_ir;
> 24d79ebc6ccec55 Sean Young 2017-07-07  166  	rcdev->driver_name = DRIVER_NAME;
> 24d79ebc6ccec55 Sean Young 2017-07-07  167  	rcdev->device_name = DEVICE_NAME;
> 24d79ebc6ccec55 Sean Young 2017-07-07  168  	rcdev->tx_ir = gpio_ir_tx;
> 24d79ebc6ccec55 Sean Young 2017-07-07  169  	rcdev->s_tx_duty_cycle = gpio_ir_tx_set_duty_cycle;
> 24d79ebc6ccec55 Sean Young 2017-07-07  170  	rcdev->s_tx_carrier = gpio_ir_tx_set_carrier;
> 24d79ebc6ccec55 Sean Young 2017-07-07  171  
> 24d79ebc6ccec55 Sean Young 2017-07-07  172  	gpio_ir->carrier = 38000;
> 24d79ebc6ccec55 Sean Young 2017-07-07  173  	gpio_ir->duty_cycle = 50;
> 24d79ebc6ccec55 Sean Young 2017-07-07 @174  	spin_lock_init(&gpio_ir->lock);

Oops, this line should've been removed. I sent the wrong patch.

I'll the send a v2 soon.

Sean

> 24d79ebc6ccec55 Sean Young 2017-07-07  175  
> 24d79ebc6ccec55 Sean Young 2017-07-07  176  	rc = devm_rc_register_device(&pdev->dev, rcdev);
> 24d79ebc6ccec55 Sean Young 2017-07-07  177  	if (rc < 0)
> 24d79ebc6ccec55 Sean Young 2017-07-07  178  		dev_err(&pdev->dev, "failed to register rc device\n");
> 24d79ebc6ccec55 Sean Young 2017-07-07  179  
> 24d79ebc6ccec55 Sean Young 2017-07-07  180  	return rc;
> 24d79ebc6ccec55 Sean Young 2017-07-07  181  }
> 24d79ebc6ccec55 Sean Young 2017-07-07  182  
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/drivers/media/rc/gpio-ir-tx.c b/drivers/media/rc/gpio-ir-tx.c
index f33b443bfa47..70d0d2baa3ae 100644
--- a/drivers/media/rc/gpio-ir-tx.c
+++ b/drivers/media/rc/gpio-ir-tx.c
@@ -19,8 +19,6 @@  struct gpio_ir {
 	struct gpio_desc *gpio;
 	unsigned int carrier;
 	unsigned int duty_cycle;
-	/* we need a spinlock to hold the cpu while transmitting */
-	spinlock_t lock;
 };
 
 static const struct of_device_id gpio_ir_tx_of_match[] = {
@@ -53,12 +51,11 @@  static int gpio_ir_tx_set_carrier(struct rc_dev *dev, u32 carrier)
 static void gpio_ir_tx_unmodulated(struct gpio_ir *gpio_ir, uint *txbuf,
 				   uint count)
 {
-	unsigned long flags;
 	ktime_t edge;
 	s32 delta;
 	int i;
 
-	spin_lock_irqsave(&gpio_ir->lock, flags);
+	local_irq_disable();
 
 	edge = ktime_get();
 
@@ -72,14 +69,11 @@  static void gpio_ir_tx_unmodulated(struct gpio_ir *gpio_ir, uint *txbuf,
 	}
 
 	gpiod_set_value(gpio_ir->gpio, 0);
-
-	spin_unlock_irqrestore(&gpio_ir->lock, flags);
 }
 
 static void gpio_ir_tx_modulated(struct gpio_ir *gpio_ir, uint *txbuf,
 				 uint count)
 {
-	unsigned long flags;
 	ktime_t edge;
 	/*
 	 * delta should never exceed 0.5 seconds (IR_MAX_DURATION) and on
@@ -95,7 +89,7 @@  static void gpio_ir_tx_modulated(struct gpio_ir *gpio_ir, uint *txbuf,
 	space = DIV_ROUND_CLOSEST((100 - gpio_ir->duty_cycle) *
 				  (NSEC_PER_SEC / 100), gpio_ir->carrier);
 
-	spin_lock_irqsave(&gpio_ir->lock, flags);
+	local_irq_disable();
 
 	edge = ktime_get();
 
@@ -128,19 +122,20 @@  static void gpio_ir_tx_modulated(struct gpio_ir *gpio_ir, uint *txbuf,
 			edge = last;
 		}
 	}
-
-	spin_unlock_irqrestore(&gpio_ir->lock, flags);
 }
 
 static int gpio_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
 		      unsigned int count)
 {
 	struct gpio_ir *gpio_ir = dev->priv;
+	unsigned long flags;
 
+	local_irq_save(flags);
 	if (gpio_ir->carrier)
 		gpio_ir_tx_modulated(gpio_ir, txbuf, count);
 	else
 		gpio_ir_tx_unmodulated(gpio_ir, txbuf, count);
+	local_irq_restore(flags);
 
 	return count;
 }