linux-kernel - Re: [PATCH] Add sc16is7x2 driver

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 for Android: free password hash cracker in your pocket

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

Message-ID: <4C8F7381.50300@iis.fraunhofer.de>
Date:	Tue, 14 Sep 2010 15:07:13 +0200
From:	Manuel Stahl <manuel.stahl@....fraunhofer.de>
To:	Andrew Morton <akpm@...ux-foundation.org>
CC:	linux-kernel@...r.kernel.org, Greg KH <greg@...ah.com>,
	linux-serial@...r.kernel.org
Subject: Re: [PATCH] Add sc16is7x2 driver

Hi Andrew,

thank you for reviewing this driver. A cleaned up patch is attached.

Am 14.09.2010 02:25, schrieb Andrew Morton:
> On Fri, 03 Sep 2010 15:11:53 +0200
> Manuel Stahl<manuel.stahl@....fraunhofer.de>  wrote:
>
>> This patch adds support for the sc16is7x2 chips.
>>
>
> The patch was pretty badly wordwrapped.

Can you point out a few such places?


>> +static unsigned int sc16is7x2_tx_empty(struct uart_port *port)
>> +{
>> +	struct sc16is7x2_channel *chan =
>> +			container_of(port, struct sc16is7x2_channel, uart);
>> +	struct sc16is7x2_chip *ts = chan->chip;
>> +	unsigned lsr;
>> +
>> +	dev_dbg(&ts->spi->dev, "%s\n", __func__);
>> +
>> +	mutex_lock(&chan->lock);
>> +	lsr = chan->lsr;
>> +	mutex_unlock(&chan->lock);
>
> It's strange to put locking around a single atomic read.  What are we
> trying to do here?
>
>> +	return lsr&  UART_LSR_TEMT ? TIOCSER_TEMT : 0;
>> +}
>> +
>>
>> ...
>>
>> +static void sc16is7x2_break_ctl(struct uart_port *port, int break_state)
>> +{
>> +	struct sc16is7x2_channel *chan =
>> +			container_of(port, struct sc16is7x2_channel, uart);
>> +	struct sc16is7x2_chip *ts = chan->chip;
>> +	unsigned ch = port->line&  0x01;
>> +	unsigned long flags;
>> +
>> +	dev_dbg(&ts->spi->dev, "%s\n", __func__);
>> +
>> +	spin_lock_irqsave(&chan->uart.lock, flags);
>> +	if (break_state == -1)
>> +		chan->lcr |= UART_LCR_SBC;
>> +	else
>> +		chan->lcr&= ~UART_LCR_SBC;
>> +	spin_unlock_irqrestore(&chan->uart.lock, flags);
>
> hm.  Above we use the mutex to protect char->lcr but here we're using a
> spinlock.

I think in the break_ctl function it's not possible to use mutexes. What 
would be the right way to protect the register cache?

>>
>> ...
>>
>> +static irqreturn_t sc16is7x2_interrupt(int irq, void *dev_id)
>> +{
>> +	struct sc16is7x2_chip *ts = dev_id;
>> +
>> +	dev_dbg(&ts->spi->dev, "%s\n", __func__);
>> +
>> +	if (!ts->force_end_work&&  !work_pending(&ts->work)&&
>> +	    !freezing(current)&&  !ts->suspending)
>> +		queue_work(ts->workqueue,&ts->work);
>> +
>> +	return IRQ_HANDLED;
>> +}
>
> The kernel has infrastructure for "threaded irqs" nowadays.  What this
> driver is doing basically reimplements that concept.  Did you consider
> using threaded IRQs directly?

Ah, I didn't know that. The irq stuff is based on the max3100 driver. 
Where can I find some example how to use the new threaded irqs?

Regards,
-- 
Dipl.-Inf. Manuel Stahl
Fraunhofer-Institut für Integrierte Schaltungen IIS
- Leistungsoptimierte Systeme -
Nordostpark 93                Telefon  +49 (0)911/58061-6419
90411 Nürnberg                Fax      +49 (0)911/58061-6398
http://www.iis.fraunhofer.de  manuel.stahl@....fraunhofer.de

View attachment "add_sc16is7x2_driver.patch" of type "text/plain" (39616 bytes)