new driver (hvcs) review request and sysfs questions

new driver (hvcs) review request and sysfs questions

[Ryan Arnold]

Greeting all,

We are writing a device driver to support IBM Power5 firmware
functionality that allows us to host the consoles of hundreds of OS
partitions on a single Linux partition without requiring hundreds of
serial ports.  The patch providing this ability is at:

http://www-124.ibm.com/linux/patches/?patch_id=1377

Rather than having a hundred virtual terminal server devices
(vty-server@) to support a hundred OS images, each vty-server@ can be
connected to the console device (vty@) of several "partner" partitions
and the vty-server@ can switch between them.  Please don't shoot the
messenger on this design, we are just implementing the device driver to
support function implemented in the firmware.

Greg K-H has said that he'd like to make comments on this driver's sysfs
design on the LKML, so the link provided earlier also contains "tree"
output of /sys on a Power5 system running this driver and the hvcs
driver's sysfs design follows in the remainder of this email.

The ppc64 vio bus registers the vty-server@ device's kobject before it
is exposed to the driver by firmware.  The sysfs entry for this kobject
natively resides in /sys/devices/vdevice/vty-server@3000000* and is
symlinked in several places in the sysfs tree, notably under the vio bus
directory in /sys/bus/vio/devices and /sys/bus/vio/drivers/hvcs/, which
is the hvcs driver's sysfs directory.

An example of the vio bus's "devices" sysfs directory is shown below.

Pow5:/sys/bus/vio/devices # ls
.               l-lan@3000000c  l-lan@30000010       vty-server@30000004
..              l-lan@3000000d  rtc@4001             vty@30000000
IBM,sp@4000     l-lan@3000000e  v-scsi@30000002
l-lan@3000000b  l-lan@3000000f  vty-server@30000003

The hvcs driver sysfs entry looks like the following:

Pow5:/sys/bus/vio/drivers/hvcs # ls
.  ..  rescan  vty-server@30000003  vty-server@30000004

By design, firmware only notifies the hvcs driver of vty-server@
lifetimes, but not changes in partner info.  Since an eServer admin can
change partner info dynamically we have provided the hvcs driver sysfs
directory /sys/bus/vio/drivers/hvcs/ with the "rescan" attribute which
will query firmware and update the partner info for all the vty-server@
vdevices that this driver manages when a '1' is written to it.  Reading
the attribute will indicate the state: 1:update in process, 0:done.

An alternate approach is to move this rescan entry into the per-device
vty-server@3000000* directory or have the entry available in both the
driver and device directories.

An example vty-server@3000000* directory looks like the follow:

Pow5:/sys/bus/vio/drivers/hvcs/vty-server@30000004 # ls 
.  ..  current_vty  detach_state  dev_node  partner_clcs  partner_vtys

The "detach_state" attribute is provided to the device prior to hvcs
device attribute additions.

Since a vty-server@ can theoretically have more than one vty@ configured
as a partner we have provided each vty-server@ sysfs dir with two
read-only attributes that provide lists of easily parsed data:
"partner_vtys" and "partner_clcs".

Reading partner_vtys (as shown below) returns a list of partner vty@
vdevices.  Note that vty@ device numbering is per-partition-unique and
the vty@ entries you see in the example below are how the vty@ vdevices
appear in the sysfs tree on their own partition (reference:
/sys/bus/vio/devices/vty@30000000 above), but we could leave off the
"vty@" portion of the ID in this entry.

Pow5:/sys/bus/vio/drivers/hvcs/vty-server@30000004 # cat partner_vtys
vty@30000000
vty@30000001
vty@30000002
vty@30000000
vty@30000000

Reading partner_clcs (as shown below) returns a list of "converged
location codes" which are composed of a system serial number followed by
"-V*" (* = target partition number) and "-C*" (* = slot of the
adapter).  This is the format given for the clc as it is returned from
firmware.  The clc could be pre-parsed by the driver and output to only
show pertinent partition and/or slot data.  The first vty@ partner
corresponds to the first clc item, the second vty@ partner to the second
clc item, etc.

Pow5:/sys/bus/vio/drivers/hvcs/vty-server@30000004 # cat partner_clcs
U9111.520.100048A-V3-C0
U9111.520.100048A-V3-C2
U9111.520.100048A-V3-C3
U9111.520.100048A-V4-C0
U9111.520.100048A-V5-C0

Currently a third read only entry resides under the vty-server@ vdevice
sysfs directory and that is "dev_node" which we use to tell us what
/dev/hvcs* entry (a dynamic major/minor tty device) we should create and
use to interact with this vty-server@.  This will disappear when I
figure out how udev will be used to create the /dev/ entries for my
vty-server@ vdevices.

A vty-server@ can be connected to a single vty@ at a time.  When there
are multiple vty@ partners available the user needs a method for
directing his vty-server@ to connect to the desired vty@ partner.

The entry, "current_vty" prints the clc of the currently selected
partner vty@ when read. The current_vty can be changed by writing a
valid partner clc to the entry. Changing the current_vty when a
vty-server@ is already connected to a vty@ does not affect the current
connection.  The change takes effect when the currently open connection
is freed.

Greg, I would appreciate if you could comment on my usage of sysfs.  I
would also appreciate additional comments on the driver and sysfs usage
from everyone else.

P.S. who is maintainer of the char device tree?

Thanks,
Ryan S. Arnold <rsa@us.ibm.com>
IBM LTC, Rochester MN

Re: new driver (hvcs) review request and sysfs questions

[Greg KH]

On Tue, Feb 24, 2004 at 06:00:26PM -0600, Ryan Arnold wrote:
> An example of the vio bus's "devices" sysfs directory is shown below.
> 
> Pow5:/sys/bus/vio/devices # ls
> .               l-lan@3000000c  l-lan@30000010       vty-server@30000004
> ..              l-lan@3000000d  rtc@4001             vty@30000000
> IBM,sp@4000     l-lan@3000000e  v-scsi@30000002
> l-lan@3000000b  l-lan@3000000f  vty-server@30000003

At first glance, why are you using text strings as part of your bus ids?
Bus ids must be unique, so it looks like you can do this by just using
the number after the '@' character, right?

Then, within each device on the bus, you can give it a "name" or "type"
if you want.  You can put the "l-lan", "rtc", "vty*" stuff there, and
not encode it in the bus id (which is not where it belongs.)

> P.S. who is maintainer of the char device tree?

Do you mean /drivers/char?  Not really anyone directly.  Do you have
some char drivers that need to get added there?  What type of drivers
are they?

thanks,

greg k-h

Re: new driver (hvcs) review request and sysfs questions

[Dave Boutcher]

On Tue, 24 Feb 2004 17:28:45 -0800, Greg KH <greg@kroah.com> wrote:

> On Tue, Feb 24, 2004 at 06:00:26PM -0600, Ryan Arnold wrote:
>> An example of the vio bus's "devices" sysfs directory is shown below.
>>
>> Pow5:/sys/bus/vio/devices # ls
>> .               l-lan@3000000c  l-lan@30000010       vty-server@30000004
>> ..              l-lan@3000000d  rtc@4001             vty@30000000
>> IBM,sp@4000     l-lan@3000000e  v-scsi@30000002
>> l-lan@3000000b  l-lan@3000000f  vty-server@30000003
>
> At first glance, why are you using text strings as part of your bus ids?
> Bus ids must be unique, so it looks like you can do this by just using
> the number after the '@' character, right?

That naming convention (e.g. l-lan@3000000c) is what is defined in the 
IEEE Standard for Boot (Initialization Configuration) Firmware spec, and 
what gets presented by Open Firmware.  It is what has been used in things 
like /proc/device-tree on the Power platforms forever, and thus was 
carried over into the sysfs definitions.

It is also true that it is unlike the representation of most other things 
in sysfs, so perhaps this is the time to change before it gets too baked 
into things.

Dave B

Re: new driver (hvcs) review request and sysfs questions

[Greg KH]

On Tue, Feb 24, 2004 at 09:12:09PM -0600, Dave Boutcher wrote:
> 
> It is also true that it is unlike the representation of most other things 
> in sysfs, so perhaps this is the time to change before it gets too baked 
> into things.

I agree.  Is there any reason we _have_ to stick with the OF names?  It
seems to me to make more sense here not to, to make it more like the
rest of the kernel.

That is, if the address after the @ is unique.  Is that always the case?

thanks,

greg k-h

Re: new driver (hvcs) review request and sysfs questions

[Benjamin Herrenschmidt]

On Wed, 2004-02-25 at 15:22, Greg KH wrote:
> On Tue, Feb 24, 2004 at 09:12:09PM -0600, Dave Boutcher wrote:
> > 
> > It is also true that it is unlike the representation of most other things 
> > in sysfs, so perhaps this is the time to change before it gets too baked 
> > into things.
> 
> I agree.  Is there any reason we _have_ to stick with the OF names?  It
> seems to me to make more sense here not to, to make it more like the
> rest of the kernel.
> 
> That is, if the address after the @ is unique.  Is that always the case?

That is the problem... I didn't check my OF spec, but I do remember
clearly cases where the "unit address" isn't unique... This happens
typically at the root of the device-tree, or with pseudo devices,
where you can have several entries with an @0 unit address. However,
I yet have to see that for things that are worth putting into sysfs ;)

One thing though is that it's only unique at a given level of
hierarchy. The Unit Address in OF has no meaning outside of the
context of the parent bus. That may be just fine for sysfs, but
if I take as an example the PCI devices, they do have a globally
unique ID here with the domain number.

Ben.

Re: device/kobject naming

[Hollis Blanchard]

On Feb 25, 2004, at 5:34 AM, Benjamin Herrenschmidt wrote:
> On Wed, 2004-02-25 at 15:22, Greg KH wrote:
>> I agree.  Is there any reason we _have_ to stick with the OF names?  
>> It
>> seems to me to make more sense here not to, to make it more like the
>> rest of the kernel.
>>
>> That is, if the address after the @ is unique.  Is that always the 
>> case?
>
> One thing though is that it's only unique at a given level of
> hierarchy. The Unit Address in OF has no meaning outside of the
> context of the parent bus. That may be just fine for sysfs, but
> if I take as an example the PCI devices, they do have a globally
> unique ID here with the domain number.

Yes, that's certainly the case. Every unit address on the virtual bus 
will be unique, but device_add() uses dev.bus_id as the kobject name, 
which is system-wide.

Apparently PCI gets away with multiple busses by encoding the domain 
and bus IDs into dev.bus_id along with the slot number. Even then, it's 
just kind of coincidence that nothing else wants to register kobjects 
with names like 0000:00:0b.0, right? Unless we want to start defining 
mandatory "domains" for every type of device and prefixing things like 
that...

At any rate, virtual IO devices effectively have just a slot number and 
nothing else. Do you really want to start registering kobjects with 
names like "30000000"? Or what about "vio:30000000", and then have it 
show up as "/sys/devices/vio/vio:30000000"? Seems redundant to me.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: device/kobject naming

[Greg KH]

On Wed, Feb 25, 2004 at 09:46:24AM -0600, Hollis Blanchard wrote:
> On Feb 25, 2004, at 5:34 AM, Benjamin Herrenschmidt wrote:
> >On Wed, 2004-02-25 at 15:22, Greg KH wrote:
> >>I agree.  Is there any reason we _have_ to stick with the OF names?  
> >>It
> >>seems to me to make more sense here not to, to make it more like the
> >>rest of the kernel.
> >>
> >>That is, if the address after the @ is unique.  Is that always the 
> >>case?
> >
> >One thing though is that it's only unique at a given level of
> >hierarchy. The Unit Address in OF has no meaning outside of the
> >context of the parent bus. That may be just fine for sysfs, but
> >if I take as an example the PCI devices, they do have a globally
> >unique ID here with the domain number.
> 
> Yes, that's certainly the case. Every unit address on the virtual bus 
> will be unique, but device_add() uses dev.bus_id as the kobject name, 
> which is system-wide.
> 
> Apparently PCI gets away with multiple busses by encoding the domain 
> and bus IDs into dev.bus_id along with the slot number. Even then, it's 
> just kind of coincidence that nothing else wants to register kobjects 
> with names like 0000:00:0b.0, right? Unless we want to start defining 
> mandatory "domains" for every type of device and prefixing things like 
> that...

No, I think you are confused.  The only thing that has to be unique in
the kobject/device name is it must be unique for the bus it is on.

So for PCI we encode the domain and bus id into .bus_id as we know it
will be unique on this system.  There is no "coincidence" involved at
all.  Same thing with USB, and all other busses.  You only have to be
unique for your specific bus.

So for this bus, that is all that is needed.  You don't have to worry
about the PCI/USB/FooBUS namespace at all.

> At any rate, virtual IO devices effectively have just a slot number and 
> nothing else. Do you really want to start registering kobjects with 
> names like "30000000"?

Why not?  We do it for every other bus type.

> Or what about "vio:30000000", and then have it show up as
> "/sys/devices/vio/vio:30000000"? Seems redundant to me.

Exactly, it is redundant.  That's why you don't need the "vio:" portion.

Does this help out?

thanks,

greg k-h

Re: device/kobject naming

[Hollis Blanchard]

On Feb 25, 2004, at 6:18 PM, Greg KH wrote:
>
> No, I think you are confused.  The only thing that has to be unique in
> the kobject/device name is it must be unique for the bus it is on.

I must be confused. device_initialize() sets the new device->kobj->kset 
to devices_subsys.kset. Then device_add() calls kobject_add(), which 
appends device->kobj to its kset. As we've already discussed, 
devices_subsys is a global list of all registered devices in the 
system, not a per-bus list.

It doesn't look like this will actually cause an immediate error (in 
that the sysfs directory created will be different per bus), but you 
could end up with two devices named "foo" in the devices_subsys list, 
even if they're on different busses.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: new driver (hvcs) review request and sysfs questions

[Dave Hansen]

--- pristine/linux-2.5/drivers/char/hvcs.c	...
+++ working/linux-2.5/drivers/char/hvcs.c	...

It's standard practice to create patches so that they apply with -p1. 
See Documentation/SubmittingPatches

Don't just indiscriminately run lindent.  It makes the code look awful
in some spots.  Remember that it's OK to exceed 80 columns in some
places, especially with things like function declarations.

in hvcs_write():

	count = min(count, (int)PAGE_SIZE);

assumes that count and PAGE_SIZE are in the same units: bytes.  If
that's true, no need for the sizeof() here:

charbuf = kmalloc(sizeof(unsigned char *) * count,  GFP_KERNEL);

hvcs_next_partner() shares code with lparcfg_write() and a bunch of the
other hypervisor calls.  Can you combine them with a function that just
checks the return codes from hcalls?  All of those switches look
redundant.   

hvcs_get_partner_info() is a bit long.  Can that while loop go into its
own function?

-- dave