netdev - Re: [PATCH v3 1/1 net] net: fec: fix kernel oops when plug/unplug cable many times

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:	Mon, 06 May 2013 12:16:04 +0200
From:	Lucas Stach <l.stach@...gutronix.de>
To:	Frank Li <lznuaa@...il.com>
Cc:	Frank Li <Frank.Li@...escale.com>,
	Francois Romieu <romieu@...zoreil.com>,
	Robert Schwebel <r.schwebel@...gutronix.de>,
	David Miller <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Fabio Estevam <festevam@...il.com>,
	Shawn Guo <shawn.guo@...aro.org>
Subject: Re: [PATCH v3 1/1 net] net: fec: fix kernel oops when plug/unplug
 cable many times

Am Montag, den 06.05.2013, 17:56 +0800 schrieb Frank Li:
> 2013/5/6 Lucas Stach <l.stach@...gutronix.de>:
> > Hi Frank,
> >
> > Am Donnerstag, den 02.05.2013, 08:55 +0800 schrieb Frank Li:
[...]
> >> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
> >> index eb43729..a367b21 100644
> >> --- a/drivers/net/ethernet/freescale/fec.h
> >> +++ b/drivers/net/ethernet/freescale/fec.h
> >> @@ -260,7 +260,8 @@ struct fec_enet_private {
> >>       int hwts_rx_en;
> >>       int hwts_tx_en;
> >>       struct timer_list time_keep;
> >> -
> >> +     struct delayed_work delay_work;
> >> +     int timeout;
> > I suspect you are going to add more variables like this in the future if
> > you are using the workqueue more extensively. This is a massive
> > pollution of the fec_enet_private struct. Please make this a bitfield
> > with a proper define for the timeout, so we can reuse one variable for
> > different tasks, or even split this out into it's own struct for the
> > delayed tasks.
> 
> Bit | & is not atomic.
> So I prefer use two variable. the real work in delay work is very
> simple for future workaround.

Ah yes, that's right. Could you then please change the type of the
variable to bool to make it clear that nothing other than true/false
should go into this?

I would still prefer to group the delayed work related things together
in one struct to make the driver more readable overall. Like this:
struct fec_delayed_work {
	struct delayed_work delay_work;
	bool timeout;
	bool future_workaround;
	bool more_delayed_things;
};
And just include this struct in fec_enet_private.

-- 
Pengutronix e.K.                           | Lucas Stach                 |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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