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]

Date:	Tue, 16 Apr 2013 09:57:45 +0200
From:	Oliver Neukum <oliver@...kum.org>
To:	Ming Lei <tom.leiming@...il.com>
Cc:	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

On Tuesday 16 April 2013 09:15:45 Ming Lei wrote:
> On Mon, Apr 15, 2013 at 11:59 PM, Dan Williams <dcbw@...hat.com> wrote:
> >
> > So, what was the general consensus on this one?  I know Oliver signed
> > off on it, but the discussion about memflags seemed to die out without a
> > specific conclusion.  davem might be looking for that conclusion before
> > moving forward with the series :)
> 
> Suggest to remove the memflags parameter, because:
> 
> - the probable issue addressed by introducing memflags is a general issue
> of all USB drivers, also very corener one, not a specific one on usbnet, and
> the issue only exists on devices with at least one mass storage interface and
> another non-mass-storage interface, and it is not considered by other USB
> drivers now.

Generally, saying that somebody else has a problem is not an argument
as long as the fix is very simple. Of course it is a corner case, but why
fail to solve it as long as the cost is extremely low?

> - usbnet_status_start() is called from either probe() or work queue scheduled
> from probe(), if we want to address the probable issue, the memflags should

No, we export this symbol. So we keep it generic if that is possible at low cost.
We cannot assume that the conditions it would be called in now, remain so.
Of course we don't add stuff needlessly, but here the fix is trivial.

> always be GFP_NOIO under the two situations, __or__ GFP_KERNEL if we
> choose to ignore the very corner case like other USB drivers.  So hardcoded
> GFP_NOIO or GFP_KERNEL should be accepted.
> 
> Oliver, do you have objections on not adding the memflags parameter now?

Yes, it is a change of almost no gain and a known problem.
It should be added with mem_flags.
 
> Williams, looks there is another problem in your patch, sorry for not
> finding it previously.  usb_autopm_get_interface() need to be called before
> submitting URB inside usbnet_status_start(), and usb_autopm_put_interface()
> need to be called after killing URB inside usbnet_status_stop(). Otherwise
> your patch doesn't work as you expected under runtime PM situation.

Again, no. This is a generic API. It may be called for devices which need
their status polled forever and we cannot block them from sleeping.
If a driver wants to block suspend while a status URB is on, it should
call the autopm method. This is the way we do it while the connection
is up.

	Regards
		Oliver

--
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