Re: [PATCH 3/5] s390: Add a mechanism to get the subchannel id.

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Tue, 11 Dec 2012 11:09:55 +0100
Alexander Graf <agraf@suse.de> wrote:

> 
> On 10.12.2012, at 10:03, Cornelia Huck wrote:
> 
> > On Sun, 9 Dec 2012 13:12:37 +0100
> > Alexander Graf <agraf@suse.de> wrote:
> > 
> >> 
> >> On 07.12.2012, at 13:29, Cornelia Huck wrote:
> >> 
> >>> This will be needed by the new virtio-ccw transport.
> >>> 
> >>> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> >>> ---
> >>> arch/s390/include/asm/ccwdev.h |  5 +++++
> >>> drivers/s390/cio/device_ops.c  | 12 ++++++++++++
> >>> 2 files changed, 17 insertions(+)
> >>> 
> >>> diff --git a/arch/s390/include/asm/ccwdev.h b/arch/s390/include/asm/ccwdev.h
> >>> index 1cb4bb3..9ad79f7 100644
> >>> --- a/arch/s390/include/asm/ccwdev.h
> >>> +++ b/arch/s390/include/asm/ccwdev.h
> >>> @@ -18,6 +18,9 @@ struct irb;
> >>> struct ccw1;
> >>> struct ccw_dev_id;
> >>> 
> >>> +/* from asm/schid.h */
> >>> +struct subchannel_id;
> >>> +
> >>> /* simplified initializers for struct ccw_device:
> >>> * CCW_DEVICE and CCW_DEVICE_DEVTYPE initialize one
> >>> * entry in your MODULE_DEVICE_TABLE and set the match_flag correctly */
> >>> @@ -226,5 +229,7 @@ int ccw_device_siosl(struct ccw_device *);
> >>> // FIXME: these have to go
> >>> extern int _ccw_device_get_subchannel_number(struct ccw_device *);
> >>> 
> >>> +extern void ccw_device_get_schid(struct ccw_device *, struct subchannel_id *);
> >>> +
> >>> extern void *ccw_device_get_chp_desc(struct ccw_device *, int);
> >>> #endif /* _S390_CCWDEV_H_ */
> >>> diff --git a/drivers/s390/cio/device_ops.c b/drivers/s390/cio/device_ops.c
> >>> index ec7fb6d..2ad832f 100644
> >>> --- a/drivers/s390/cio/device_ops.c
> >>> +++ b/drivers/s390/cio/device_ops.c
> >>> @@ -763,6 +763,18 @@ _ccw_device_get_subchannel_number(struct ccw_device *cdev)
> >>> 	return cdev->private->schid.sch_no;
> >>> }
> >>> 
> >>> +/**
> >>> + * ccw_device_get_schid - obtain a subchannel id
> >>> + * @cdev: device to obtain the id for
> >>> + * @schid: where to fill in the values
> >>> + */
> >>> +void ccw_device_get_schid(struct ccw_device *cdev, struct subchannel_id *schid)
> >> 
> >> If you make this
> >> 
> >> u32 ccw_device_get_schid(struct ccw_device *cdev)
> >> 
> >> and you return the u32 (architected) value of the schid, not the internal struct, then you can save yourself the struct export (patch 2/5) and the ugly cast in patch 4/5 to get the u32 value.
> > 
> > I really prefer using the structure instead.
> > 
> > Moreover, there's a patch based on this that switches non-kvm users to
> > this new interface (getting rid of an old, broken interface) already
> > queued in the linux-s390 tree AFAIK.
> 
> Well, then base on top of that patch and add another interface that allows you to receive a schid as integer value. Randomly casting structs to u32 in random code across the tree is just pure ugly.

We seem to have different ideas of 'ugly' :)

I really prefer to have a function called ccw_device_get_schid()
provide a struct subchannel_id and not a u32 - a subchannel id is,
after all, what the caller asks for. The fact that a subchannel id may
fit into a u32 is an implementation detail, as well as the fact that an
instruction takes a 32 bit value containing a subchannel id. Passing
around a 32 bit value instead of a subchannel id basically discards the
information that we're dealing with a subchannel id and not with a
random 32 bit value.

> 
> An alternative would be to create a union around the struct and to return that one, so that you can access the u32 value or the struct value depending on the user's needs. That way the u32 cast is part of the API and not implicit, as it would be today.

What's the API here? I think we have two parts:

- The API to get the current subchannel id for a ccw device. For the
reasons above, I think this should use struct subchannel_id.

- The hardware/hypervisor API for addressing a subchannel, which
basically requires that a register contains a subchannel id. The easiest
way to do this is to cast to an integer. I don't see any need to play
games with unions here, after all we're using C and not Pascal ;)