LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jan-Bernd Themann <ossthema@de.ibm.com>
To: John Rose <johnrose@austin.ibm.com>
Cc: Jeff Garzik <jeff@garzik.org>,
	Christoph Raisch <RAISCH@de.ibm.com>,
	Jan-Bernd Themann <themann@de.ibm.com>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	linux-ppc <linuxppc-dev@ozlabs.org>,
	Marcus Eder <MEDER@de.ibm.com>, Thomas Klein <tklein@de.ibm.com>,
	stefan.roscher@de.ibm.com, netdev <netdev@vger.kernel.org>,
	Nathan Fontenot <nfont@austin.ibm.com>
Subject: Re: [PATCH 2.6.21-rc1] ehea: dynamic add / remove port
Date: Fri, 16 Feb 2007 16:06:52 +0100	[thread overview]
Message-ID: <200702161606.52789.ossthema@de.ibm.com> (raw)
In-Reply-To: <1171491906.30473.3.camel@sinatra.austin.ibm.com>

Hi,

I agree with most points. Here the new design proposal:

On Wednesday 14 February 2007 23:25, John Rose wrote:
> Hi-
> 
> A few high level comments, then some really insignificant ones.
> 
> First, is there a reason why we shouldn't have a sysfs entry/kobject for
> each logical port?  How is it possible to determine, from the adapter
> sysfs directory, the current number of ports for that adapter?  A port
> sysfs directory could include attributes like the OF path to the port,
> the state of the port, etc etc.

I think it is not necessary to have a special entry/kobject for each logical
port. I suggest we use SET_NETDEV_DEV to create links to all ethernet devices
that represent each a logical port. This should be in sync with all other 
ethernet drivers. Port attributes like the "logical port id" that might be 
need by the userspace application can be added to the net:ethX entry
(link created with SET_NETDEV_DEV) as additional attributes. 
Thus we have a simple and consistent way to determine 
all currently available network interfaces (logical ports) and its
corresponding IDs.

> 
> Second, the probe and remove functions do not communicate whether an add
> or remove was successful.  Combine this with the lack of port
> information in the adapter sysfs directory, and the userspace tool has
> no way of verifying a dynamic add/remove.

True. I suggest we return error codes (-EIO / -EINVAL) in case the adding
or removing of ports fails. This is possible as we know instantly if the
operation failed or not. It is a synchronus operation.


> 
> +		ehea_info("%s -> logial port id #%d",
> 
> Spelling :)
true, fixed

> 
>  	if (port_setup_ok)
> -		ret = 0;	/* At least some ports are setup correctly */
> +		return 0;	/* At least some ports are setup correctly */
>  	else
> -		ret = -EINVAL;
> +		return -EINVAL;
> 
> The else is unnecessary.

agreed, fixed

> 
>  static int __devexit ehea_remove(struct ibmebus_dev *dev)
>  {
>  	struct ehea_adapter *adapter = dev->ofdev.dev.driver_data;
>  	u64 hret;
>  	int i;
> 
> -	for (i = 0; i < adapter->num_ports; i++)
> +	for (i = 0; i < EHEA_MAX_PORTS; i++)
>  		if (adapter->port[i]) {
>  			ehea_shutdown_single_port(adapter->port[i]);
>  			adapter->port[i] = NULL;
>  		}
> 
> Else break?
no break wanted here as "remove_port" might cause situations where
not all currently available ports are in a row

Regards,
Jan-Bernd

  parent reply	other threads:[~2007-02-16 15:11 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-02-14 14:36 Jan-Bernd Themann
2007-02-14 22:25 ` John Rose
2007-02-15 17:04   ` John Rose
2007-02-16 15:06   ` Jan-Bernd Themann [this message]
2007-02-16 15:51     ` John Rose

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=200702161606.52789.ossthema@de.ibm.com \
    --to=ossthema@de.ibm.com \
    --cc=MEDER@de.ibm.com \
    --cc=RAISCH@de.ibm.com \
    --cc=jeff@garzik.org \
    --cc=johnrose@austin.ibm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@ozlabs.org \
    --cc=netdev@vger.kernel.org \
    --cc=nfont@austin.ibm.com \
    --cc=stefan.roscher@de.ibm.com \
    --cc=themann@de.ibm.com \
    --cc=tklein@de.ibm.com \
    --subject='Re: [PATCH 2.6.21-rc1] ehea: dynamic add / remove port' \
    /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

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