From: Andrew Morton <akpm@linux-foundation.org>
To: "Bounine, Alexandre" <Alexandre.Bounine@idt.com>
Cc: linux-kernel@vger.kernel.org, Thomas Moll <thomas.moll@sysgo.com>,
linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v2 03/10] RapidIO: Use stored ingress port number instead of register read
Date: Mon, 20 Sep 2010 12:17:25 -0700 [thread overview]
Message-ID: <20100920121725.66bb96d0.akpm@linux-foundation.org> (raw)
In-Reply-To: <0CE8B6BE3C4AD74AB97D9D29BD24E55201303D0E@CORPEXCH1.na.ads.idt.com>
On Mon, 20 Sep 2010 07:31:22 -0700
"Bounine, Alexandre" <Alexandre.Bounine@idt.com> wrote:
> Andrew Morton <akpm@linux-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?).
next prev parent reply other threads:[~2010-09-20 19:18 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-09-14 14:59 [PATCH v2 0/10] RapidIO: Set of patches to add Gen2 switches Alexandre Bounine
2010-09-14 14:59 ` [PATCH v2 01/10] RapidIO: Fix RapidIO sysfs hierarchy Alexandre Bounine
2010-09-14 22:04 ` Andrew Morton
2010-09-15 14:04 ` Bounine, Alexandre
2010-09-14 14:59 ` [PATCH v2 02/10] RapidIO:powerpc/85xx: Modify RIO port-write interrupt handler Alexandre Bounine
2010-09-14 14:59 ` [PATCH v2 03/10] RapidIO: Use stored ingress port number instead of register read Alexandre Bounine
2010-09-14 22:12 ` Andrew Morton
2010-09-15 19:28 ` Bounine, Alexandre
2010-09-20 14:31 ` Bounine, Alexandre
2010-09-20 19:17 ` Andrew Morton [this message]
2010-09-20 19:49 ` Bounine, Alexandre
2010-09-20 20:40 ` Micha Nelissen
2010-10-01 20:46 ` Bounine, Alexandre
2010-09-14 14:59 ` [PATCH v2 04/10] RapidIO: Add relation links between RIO device structures Alexandre Bounine
2010-09-14 14:59 ` [PATCH v2 05/10] RapidIO: Add default handler for error-stopped state Alexandre Bounine
2010-09-14 14:59 ` [PATCH v2 06/10] RapidIO: Modify sysfs initialization for switches Alexandre Bounine
2010-09-14 22:10 ` Andrew Morton
2010-09-15 13:38 ` Bounine, Alexandre
2010-09-14 14:59 ` [PATCH v2 07/10] RapidIO: Add handling of orphan port-write message Alexandre Bounine
2010-09-14 14:59 ` [PATCH v2 08/10] RapidIO: Add device access check into the enumeration Alexandre Bounine
2010-09-14 14:59 ` [PATCH v2 09/10] RapidIO: Add support for IDT CPS Gen2 switches Alexandre Bounine
2010-09-14 22:20 ` Andrew Morton
2010-09-15 15:30 ` Bounine, Alexandre
2010-09-15 18:27 ` Anderson, Trevor
2010-09-15 18:52 ` Bounine, Alexandre
2010-09-15 19:13 ` Anderson, Trevor
2010-09-14 14:59 ` [PATCH v2 10/10] RapidIO: Add handling of redundant routes Alexandre Bounine
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20100920121725.66bb96d0.akpm@linux-foundation.org \
--to=akpm@linux-foundation.org \
--cc=Alexandre.Bounine@idt.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=thomas.moll@sysgo.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).