On Wed, 12 Dec 2012 01:33:46 +0100 Alexander Graf <agraf@xxxxxxx> wrote: > > On 11.12.2012, at 13:34, Cornelia Huck wrote: > > > On Tue, 11 Dec 2012 11:09:55 +0100 > > Alexander Graf <agraf@xxxxxxx> wrote: > > > >> > >> On 10.12.2012, at 10:03, Cornelia Huck wrote: > >> > >>> On Sun, 9 Dec 2012 13:12:37 +0100 > >>> Alexander Graf <agraf@xxxxxxx> 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@xxxxxxxxxx> > >>>>> --- > >>>>> 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. > > Fine with me. > > But as you are mentioning above the fact that a subchannel id is a u32 internally is an implementation detail. Thus casting a struct schid * to a u32 * is violating that exact rule. > > > - 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 ;) > > Then just create a small inline function in the header file that converts a struct schid * to u32. I want to make sure we keep implementation details in files that deal with the implementation details, not in files that deal with using the interfaces. > I checked what the common I/O layer does for the I/O instructions in drivers/s390/cio/ioasm.h and think that's really the best solution: pass the subchannel id directly in the register. No information is lost, no confusing casts, and I think the code looks nicer. Updated guest patch set coming now. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
