On Tue, 11 Dec 2012 11:09:55 +0100 Alexander Graf <[email protected]> wrote:
> > On 10.12.2012, at 10:03, Cornelia Huck wrote: > > > On Sun, 9 Dec 2012 13:12:37 +0100 > > Alexander Graf <[email protected]> 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 <[email protected]> > >>> --- > >>> 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 ;) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
