linux-kernel - Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <201103181605.36877.arnd@arndb.de>
Date:	Fri, 18 Mar 2011 16:05:36 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	Waldemar.Rymarkiewicz@...to.com
Cc:	alan@...rguk.ukuu.org.uk, linux-i2c@...r.kernel.org,
	sameo@...ux.intel.com, linux-kernel@...r.kernel.org,
	hthebaud@...idefr.com, matti.j.aaltonen@...ia.com
Subject: Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

On Friday 18 March 2011, Waldemar.Rymarkiewicz@...to.com wrote:
> >
> >Ermm nope.. why do we have do nothing ioctls ?
> >
> 
> onfc stack requires those ones, but they are only valid for a specific test enviroment.
> This should not be a case for driver and the stack should care about it if it needs
> this. Then will remove it.

The way this normally works is to figure out the correct
way to implement the kernel driver first and then write
a user space stack around it, not the other way round...
  
> >> +
> >> +    mutex_lock(&info->rx_mutex);
> >> +    info->irq_state = 1;
> >> +    mutex_unlock(&info->rx_mutex);
> >
> >Would it not be lighter to use atomic bit ops ?
> 
> Do you mean  in order to remove rx_mutex? 
> 
> mutex_lock(&info->rx_mutex);
> atomic_set(info->irq_state ,1);
> mutex_unlock(&info->rx_mutex);
> 
> looks a bit strange. I still need the rx_mutex to protect irq_state while reading i2c.
> 
>         mutex_lock(&info->rx_mutex);
>         ret = i2c_master_recv(client, info->buf, info->buflen);
>         info->irq_state = 0;
>         mutex_unlock(&info->rx_mutex);

What Alan meant was set_bit/test_and_clear_bit here, not atomic_*().
As I mentioned in the other mail, the way you check the flag is
probably still racy with the current mutex use.

Note that the interrupt handler doesn't do much at all, so it's probably
a good idea to use a regular (non-threaded) handler here to reduce
the overhead, once you have removed the mutex.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/