netdev - Re: [PATCH 1/2 v5] usbnet: allow status interrupt URB to always be active

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: <87ppy1wfhq.fsf@nemi.mork.no>
Date:	Thu, 11 Apr 2013 12:04:01 +0200
From:	Bjørn Mork <bjorn@...k.no>
To:	Ming Lei <tom.leiming@...il.com>
Cc:	Oliver Neukum <oliver@...kum.org>, Dan Williams <dcbw@...hat.com>,
	Elina Pasheva <epasheva@...rrawireless.com>,
	Network Development <netdev@...r.kernel.org>,
	linux-usb <linux-usb@...r.kernel.org>,
	Rory Filer <rfiler@...rrawireless.com>,
	Phil Sutter <phil@....cc>
Subject: Re: [PATCH 1/2 v5] usbnet: allow status interrupt URB to always be active

Ming Lei <tom.leiming@...il.com> writes:

> On Thu, Apr 11, 2013 at 4:06 PM, Bjørn Mork <bjorn@...k.no> wrote:
>> Oliver Neukum <oliver@...kum.org> writes:
>>
>> My immediate thought was that someone also might want to use this new
>> API from atomic context, e.g. calling it directly from an URB callback.
>
> I am wondering it is a valid use case, and if there is one URB submitted,
> the interrupt URB for status has been submitted already, hasn't it?

It might not be valid.

>> But that is of course not possible taking a mutex.  Could the lock
>> preventing interrupt_count maybe be a spinlock instead?  Or am I on the
>> completely wrong track here?
>
> Also it is a bit odd that the 'start' API is allowed in atomic context, but
> the 'stop' API isn't allowed, and it is very easy to cause unbalanced counter.

Yes, that's a valid point.  Just a random thought popping out :)

For the record: I believe the v5 patch as posted really is fine without
any changes.

>> In any case, I don't see the point unnecessarily limiting the API by
>> dropping the memflags.  What possible problem would that solve?
>
> If you think 'start' API should be called in atomic context, the memflags
> should be always 'GFP_ATOMIC'. I let Oliver explain why GFP_NOIO
> is needed in other cases.

Again: What problem are you attempting to solve by removing the
mem_flags from the API?

I think you are turning this the wrong way around. Please explain why
there are no use cases where different flags are needed.  You seem to be
only concerned about the resume case. This API is not limited to
resuming. We pass mem_flags around all the time. It's the common thing
to do in any API where allocations may be required.


Bjørn
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html