On 30.10.2012, at 15:35, Cornelia Huck <cornelia.huck@xxxxxxxxxx> wrote:
> On Tue, 30 Oct 2012 15:05:23 +0100
> Alexander Graf <agraf@xxxxxxx> wrote:
>
>>
>>
>> On 30.10.2012, at 15:00, Cornelia Huck <cornelia.huck@xxxxxxxxxx> wrote:
>>
>>> On Tue, 30 Oct 2012 14:41:46 +0100
>>> Alexander Graf <agraf@xxxxxxx> wrote:
>>>
>>>> On 10/30/2012 02:03 PM, Cornelia Huck wrote:
>>>>> On Mon, 29 Oct 2012 19:37:10 +0100
>>>>> Alexander Graf<agraf@xxxxxxx> wrote:
>>>>>
>>>>>> On 29.10.2012, at 19:34, Cornelia Huck wrote:
>>>>>>
>>>>>>> On Mon, 29 Oct 2012 19:12:54 +0100
>>>>>>> Alexander Graf<agraf@xxxxxxx> wrote:
>>>>>>>
>>>>>>>> On 29.10.2012, at 14:07, Cornelia Huck wrote:
>>>>>>>>> +static void virtio_ccw_kvm_notify(struct virtqueue *vq)
>>>>>>>>> +{
>>>>>>>>> + struct virtio_ccw_vq_info *info = vq->priv;
>>>>>>>>> + struct virtio_ccw_device *vcdev;
>>>>>>>>> + struct subchannel_id schid;
>>>>>>>>> + __u32 reg2;
>>>>>>>>> +
>>>>>>>>> + vcdev = to_vc_device(info->vq->vdev);
>>>>>>>>> + ccw_device_get_schid(vcdev->cdev,&schid);
>>>>>>>>> + reg2 = *(__u32 *)&schid;
>>>>>>>> That cast looks quite ugly. Can't you just access the field in there you need? Or if it's multiple fields do a union over them? Or assemble them by hand in C?
>>>>>>> I think the cast looks less ugly than using a union to morph it around.
>>>>>>> I want the schid with all fields filled out anyway, since this is what
>>>>>>> identifies the subchannel.
>>>>>> How about a helper function that returns a u32 for a struct subchannel_id in arch/s390/include/asm/schid.h then?
>>>>> This would just move the cast around, no? I don't think that would
>>>>> improve readability.
>>>>
>>>> It would take it from the user of that struct close to the definition of
>>>> the struct. The fact that it's an ugly cast by then is an implementation
>>>> detail. This way it's a public API.
>>>
>>> But API-wise, returning a struct subchannel_id is cleaner - we don't
>>> want "u32 associated with that device", but "subchannel identifier for
>>> that device". The fact that we want to stuff it into an integer for the
>>> hypercall is the implementaton detail.
>>
>> The subchannel id is not an implementation detail ;)
>
> And that's why I think it should stay the way it is :)
I mean the way it's encoded in that u32 is very important. If the struct happens to match, great. But that's only guaranteed because the struct is laid out the way it is.
But since this is generic ccw code, I'd leave the final call here to the respective maintainer ;)
Alex--
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
