On Sat, Sep 05, 2015 at 01:12:01PM +0100, Salah Triki wrote:
> poll_tick is declared global, so dgnc_driver_pollrate_* need to
> take the lock dgnc_poll_lock before accessing to this variable.
Really? The scope of a variable doesn't matter if a lock is needed for
it or not.
And this patch doesn't really do anything at all, I don't understand why
it is needed, please explain.
> dgnc_poll_lock
> the appropriate lock, since it is intended for poll scheduling and dgnc_poll_tick
> contains the poll rate. dgnc_poll_lock needs to be declared not static and
> extern in order to be visible for dgnc_driver_pollrate_*.
>
> Signed-off-by: Salah Triki <salah.triki@xxxxxxx>
> ---
> drivers/staging/dgnc/dgnc_driver.c | 2 +-
> drivers/staging/dgnc/dgnc_driver.h | 1 +
> drivers/staging/dgnc/dgnc_sysfs.c | 18 ++++++++++++++++--
> 3 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/dgnc/dgnc_driver.c b/drivers/staging/dgnc/dgnc_driver.c
> index 7546aff..f03e8b3 100644
> --- a/drivers/staging/dgnc/dgnc_driver.c
> +++ b/drivers/staging/dgnc/dgnc_driver.c
> @@ -64,6 +64,7 @@ static const struct file_operations dgnc_BoardFops = {
> uint dgnc_NumBoards;
> struct dgnc_board *dgnc_Board[MAXBOARDS];
> DEFINE_SPINLOCK(dgnc_global_lock);
> +DEFINE_SPINLOCK(dgnc_poll_lock);
> uint dgnc_Major;
> int dgnc_poll_tick = 20; /* Poll interval - 20 ms */
>
> @@ -75,7 +76,6 @@ static struct class *dgnc_class;
> /*
> * Poller stuff
> */
> -static DEFINE_SPINLOCK(dgnc_poll_lock); /* Poll scheduling lock */
> static ulong dgnc_poll_time; /* Time of next poll */
> static uint dgnc_poll_stop; /* Used to tell poller to stop */
> static struct timer_list dgnc_poll_timer;
> diff --git a/drivers/staging/dgnc/dgnc_driver.h b/drivers/staging/dgnc/dgnc_driver.h
> index 06ece51..6784d81 100644
> --- a/drivers/staging/dgnc/dgnc_driver.h
> +++ b/drivers/staging/dgnc/dgnc_driver.h
> @@ -389,6 +389,7 @@ struct channel_t {
> */
> extern uint dgnc_Major; /* Our driver/mgmt major */
> extern int dgnc_poll_tick; /* Poll interval - 20 ms */
> +extern spinlock_t dgnc_poll_lock; /* Poll scheduling lock */
> extern spinlock_t dgnc_global_lock; /* Driver global spinlock */
> extern uint dgnc_NumBoards; /* Total number of boards */
> extern struct dgnc_board *dgnc_Board[MAXBOARDS]; /* Array of board structs */
> diff --git a/drivers/staging/dgnc/dgnc_sysfs.c b/drivers/staging/dgnc/dgnc_sysfs.c
> index 44db870..d9d3d43 100644
> --- a/drivers/staging/dgnc/dgnc_sysfs.c
> +++ b/drivers/staging/dgnc/dgnc_sysfs.c
> @@ -50,17 +50,31 @@ static DRIVER_ATTR(maxboards, S_IRUSR, dgnc_driver_maxboards_show, NULL);
>
> static ssize_t dgnc_driver_pollrate_show(struct device_driver *ddp, char *buf)
> {
> - return snprintf(buf, PAGE_SIZE, "%dms\n", dgnc_poll_tick);
> + unsigned long flags;
> + int tick;
> +
> + spin_lock_irqsave(&dgnc_poll_lock, flags);
Why irqsave? You are never grabbing this lock from within an irq.
I don't think you understand how Linux kernel locks work, or how/where
they are needed. Please do a bit more research before creating patches
like this.
greg k-h
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
