linux-kernel - Re: [PATCH v2 03/10] RapidIO: Use stored ingress port number instead of register read

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:	Mon, 20 Sep 2010 12:17:25 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	"Bounine, Alexandre" <Alexandre.Bounine@....com>
Cc:	<linux-kernel@...r.kernel.org>, <linuxppc-dev@...ts.ozlabs.org>,
	"Thomas Moll" <thomas.moll@...go.com>,
	"Matt Porter" <mporter@...nel.crashing.org>,
	"Li Yang" <leoli@...escale.com>,
	"Kumar Gala" <galak@...nel.crashing.org>,
	"Micha Nelissen" <micha@...i.hopto.org>
Subject: Re: [PATCH v2 03/10] RapidIO: Use stored ingress port number
 instead of register read

On Mon, 20 Sep 2010 07:31:22 -0700
"Bounine, Alexandre" <Alexandre.Bounine@....com> wrote:

> Andrew Morton <akpm@...ux-foundation.org> wrote:
> > 
> > > @@ -219,6 +219,7 @@ struct rio_net {
> > >  /**
> > >   * struct rio_switch - RIO switch info
> > >   * @node: Node in global list of switches
> > > + * @rdev: Associated RIO device structure
> > >   * @switchid: Switch ID that is unique across a network
> > >   * @hopcount: Hopcount to this switch
> > >   * @destid: Associated destid in the path
> > > @@ -234,6 +235,7 @@ struct rio_net {
> > >   */
> > >  struct rio_switch {
> > >  	struct list_head node;
> > > +	struct rio_dev *rdev;
> > >  	u16 switchid;
> > >  	u16 hopcount;
> > >  	u16 destid;
> > 
> > What is the locking for rdev?
> 
> This question prompted me consider the following change:
> 
> Because the rio_switch structure (in current implementation) is a
> dynamically allocated part of rio_dev, I think it may be simpler to
> combine them into single allocation by using zero length array
> declaration:
> 
> struct rio_dev {
>     struct list_head global_list;
>     struct list_head net_list;
>     .....
>     ..... rest of rio_dev
>     .....
>     struct rio_switch switch[0];
> }
> 
> This will remove extra memory allocation, remove overlapping structure
> members and clean code sections like one shown below:
> 
> 	u8 hopcount = 0xff;
> 	u16 destid = rdev->destid;
> 
> 	if (rdev->rswitch) {
> 		destid = rdev->rswitch->destid;
> 		hopcount = rdev->rswitch->hopcount;
> 	}    
> 
> And this looks better aligned with RapidIO definitions - both: endpoints
> and switches are RIO devices. The current implementation of rio_dev
> somewhat separates rio_switch from its common part (this is why I have
> added that link into rio_switch). We may keep using the pointer to
> associated rio_dev, but reworking the rio_dev structure seems better way
> to me.
> 
> Please, let me know what do you think about this conversion. And if
> there are no objections I will make a patch.
> 

If you say so ;)

The "variable length array at the end of the struct" thing is pretty
commonly used and works well.  As long as we never want to change the
number of switches on the fly (hotplug?).

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