RE: [PATCH v2 03/10] RapidIO: Use stored ingress port number instead of register read

2010-09-20 Thread Bounine, Alexandre 
Andrew Morton a...@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.

Alex.
 
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2010-09-20 Thread Andrew Morton 
On Mon, 20 Sep 2010 07:31:22 -0700
Bounine, Alexandre alexandre.boun...@idt.com wrote:

 Andrew Morton a...@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?).

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

RE: [PATCH v2 03/10] RapidIO: Use stored ingress port number instead of register read

2010-09-20 Thread Bounine, Alexandre 
Andrew Morton a...@linux-foundation.org wrote:
 
 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?).

This is expected to be a strange array - its size can be 0 or 1 only.
No number of switches in that array should be changed on the fly.
In the hotplug situation entire rio_dev structure should be added (or
removed).
RIO device can be endpoint or switch. If a RIO device is a switch - the
rio_switch
structure will be added at the end.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2010-09-20 Thread Micha Nelissen 

Hi Alex,

Bounine, Alexandre wrote:

struct rio_dev {
struct list_head global_list;
struct list_head net_list;
.
. rest of rio_dev
.
struct rio_switch switch[0];
}


It makes sense to let rio_dev structures point to the switch they are 
attached to. That can be useful in various situations, but is not 
possible with this setup.


If a rio_dev is a switch then rdev-rswitch-rdev == rdev holds.


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


Note that it is possible for rdev-destid to differ from 
rdev-rswitch-destid even if rswitch-rdev == rdev (for non-hosts i.e. 
agents). rswitch-destid is the destid by which we discovered the switch 
(and can reach it) but rdev-destid is the actual id given to the switch.


Micha
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2010-09-14 Thread Andrew Morton 
On Tue, 14 Sep 2010 10:59:16 -0400
Alexandre Bounine alexandre.boun...@idt.com 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?  In other patches I see pointer chases
with no obvious locking against concurrent changes?
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev