linux-kernel - Re: [spi-devel-general] [PATCH RESEND] max3100 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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Thu, 25 Sep 2008 09:07:54 +0200
From:	chri <chripell@...il.com>
To:	"David Brownell" <david-b@...bell.net>
Cc:	spi-devel-general@...ts.sourceforge.net,
	linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
	alan@...rguk.ukuu.org.uk, linux-serial@...r.kernel.org
Subject: Re: [spi-devel-general] [PATCH RESEND] max3100 driver

On Thu, Sep 25, 2008 at 6:56 AM, David Brownell <david-b@...bell.net> wrote:
>
> Small suggestion:  next time you resend this, make sure
> that $SUBJECT mentions it's a UART driver.  Maybe even that
> it's a SPI UART driver.  That should help get more comments.
>

OK!

>
> This is a bit picky, but it's the first thing I noticed when
> scanning the patch ... wierd comment layout!  Either indent
> those all, or (better) convert to kerneldoc style.

I will have a look at kerneldoc and do the change. I'm waiting for a
minor number in "Low-density serial ports" before resending the
modifications asked by Andrew and Alan, so I will do this too. I just
trusted M-q in emacs for the comment layout.

>
> Potentially less picky:  probe() doesn't lock max3100s[],
> neither does remove(), and in fact there seems to be no
> lock for that table.  Which suggests trouble in cases like

I (wrongly!) was assuming that probing of devices is serialized. I
will add the lock.

>
> And is that workqueue single threaded?
>

it's freezeabe and (looking at include/linux/workqueue.) it implies
that it's single-threaded. This is important because I don't do
locking since I presume all the I/O to the MAX3100 is done in just one
workqueue. When I do I/O in other places (suspend for example) I
assume that the worqueue is friezed so not running.

Best regards and thanks for the review,

-- 
Christian Pellegrin, see http://www.evolware.org/chri/
"Real Programmers don't play tennis, or any other sport which requires
you to change clothes. Mountain climbing is OK, and Real Programmers
wear their climbing boots to work in case a mountain should suddenly
spring up in the middle of the computer room."
--
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/