Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1660191 > unrolled thread

Re: [RFC PATCH 6/8] firmware: arm_scmi: add initial support for power protocol

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH 6/8] firmware: arm_scmi: add initial support for power protocol Arnd Bergmann <arnd@arndb.de> - 2017-06-07 22:40 +0200
    Re: [RFC PATCH 6/8] firmware: arm_scmi: add initial support for power  protocol Sudeep Holla <sudeep.holla@arm.com> - 2017-06-08 11:40 +0200
      Re: [RFC PATCH 6/8] firmware: arm_scmi: add initial support for power protocol Arnd Bergmann <arnd@arndb.de> - 2017-06-08 13:10 +0200
        Re: [RFC PATCH 6/8] firmware: arm_scmi: add initial support for power  protocol Sudeep Holla <sudeep.holla@arm.com> - 2017-06-08 13:20 +0200

#1660191 — Re: [RFC PATCH 6/8] firmware: arm_scmi: add initial support for power protocol

On Wed, Jun 7, 2017 at 6:10 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:

> +struct scmi_msg_resp_power_attributes {
> +       __le16 num_domains;
> +       __le16 reserved;
> +       __le32 stats_addr_low;
> +       __le32 stats_addr_high;
> +       __le32 stats_size;
> +} __packed;
> +
> +struct scmi_msg_resp_power_domain_attributes {
> +       __le32 flags;
> +#define SUPPORTS_STATE_SET_NOTIFY(x)   ((x) & BIT(31))
> +#define SUPPORTS_STATE_SET_ASYNC(x)    ((x) & BIT(30))
> +#define SUPPORTS_STATE_SET_SYNC(x)     ((x) & BIT(29))
> +           u8 name[SCMI_MAX_STR_SIZE];
> +} __packed;

I think it would be better to leave out the __packed here, which
can lead to rather inefficient code. It's only really a problem when
building with -mstrict-align, but it's better to write code in a way that
doesn't rely on that.

> +static int
> +scmi_power_domain_attributes_get(struct scmi_handle *handle, u32 domain,
> +                                struct power_dom_info *dom_info)
> +{
> +       int ret;
> +       struct scmi_xfer *t;
> +       struct scmi_msg_resp_power_domain_attributes *attr;
> +
> +       ret = scmi_one_xfer_init(handle, POWER_DOMAIN_ATTRIBUTES,
> +                                SCMI_PROTOCOL_POWER, sizeof(domain),
> +                                sizeof(*attr), &t);
> +       if (ret)
> +               return ret;
> +
> +       *(__le32 *)t->tx.buf = cpu_to_le32(domain);
> +       attr = (struct scmi_msg_resp_power_domain_attributes *)t->rx.buf;

It seems you require a similar pattern in each caller of scmi_one_xfer_init(),
but it seems a little clumsy to always require those casts, so maybe there
is a nicer way to do this. How about making scmi_one_xfer_init() act
as an allocation function and having it return the buffer or a PTR_ERR?

It also seems odd to have it named 'init' but actually allocate the scmi_xfer
structure rather than filling a local variable that gets passed by reference.

       Arnd

[toc] | [next] | [standalone]

#1660985 — Re: [RFC PATCH 6/8] firmware: arm_scmi: add initial support for power protocol

On 07/06/17 21:38, Arnd Bergmann wrote:
> On Wed, Jun 7, 2017 at 6:10 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> 
>> +struct scmi_msg_resp_power_attributes {
>> +       __le16 num_domains;
>> +       __le16 reserved;
>> +       __le32 stats_addr_low;
>> +       __le32 stats_addr_high;
>> +       __le32 stats_size;
>> +} __packed;
>> +
>> +struct scmi_msg_resp_power_domain_attributes {
>> +       __le32 flags;
>> +#define SUPPORTS_STATE_SET_NOTIFY(x)   ((x) & BIT(31))
>> +#define SUPPORTS_STATE_SET_ASYNC(x)    ((x) & BIT(30))
>> +#define SUPPORTS_STATE_SET_SYNC(x)     ((x) & BIT(29))
>> +           u8 name[SCMI_MAX_STR_SIZE];
>> +} __packed;
> 
> I think it would be better to leave out the __packed here, which
> can lead to rather inefficient code. It's only really a problem when
> building with -mstrict-align, but it's better to write code in a way that
> doesn't rely on that.
> 

I assume you are referring to above structure only and not general
across all the structures ? I will have a look at this one.

>> +static int
>> +scmi_power_domain_attributes_get(struct scmi_handle *handle, u32 domain,
>> +                                struct power_dom_info *dom_info)
>> +{
>> +       int ret;
>> +       struct scmi_xfer *t;
>> +       struct scmi_msg_resp_power_domain_attributes *attr;
>> +
>> +       ret = scmi_one_xfer_init(handle, POWER_DOMAIN_ATTRIBUTES,
>> +                                SCMI_PROTOCOL_POWER, sizeof(domain),
>> +                                sizeof(*attr), &t);
>> +       if (ret)
>> +               return ret;
>> +
>> +       *(__le32 *)t->tx.buf = cpu_to_le32(domain);
>> +       attr = (struct scmi_msg_resp_power_domain_attributes *)t->rx.buf;
> 
> It seems you require a similar pattern in each caller of scmi_one_xfer_init(),
> but it seems a little clumsy to always require those casts, so maybe there
> is a nicer way to do this. How about making scmi_one_xfer_init() act
> as an allocation function and having it return the buffer or a PTR_ERR?
> 

Yes I agree it doesn't looks all nice. I have changed these few times
while developing and then thought it's better to get some suggestions. I
am open to any suggestions that will help to make these nicer.

> It also seems odd to have it named 'init' but actually allocate the scmi_xfer
> structure rather than filling a local variable that gets passed by reference.
> 

It does initialise but partially. scmi_one_xfer_get does pure allocation
while scmi_one_xfer_init initialise header variables and also tx/rx
size. But if you think it's odd, I will looks at ways to make it better.

-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

#1661060

On Thu, Jun 8, 2017 at 11:39 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 07/06/17 21:38, Arnd Bergmann wrote:
>> On Wed, Jun 7, 2017 at 6:10 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
>>> +struct scmi_msg_resp_power_attributes {
>>> +       __le16 num_domains;
>>> +       __le16 reserved;
>>> +       __le32 stats_addr_low;
>>> +       __le32 stats_addr_high;
>>> +       __le32 stats_size;
>>> +} __packed;
>>> +
>>> +struct scmi_msg_resp_power_domain_attributes {
>>> +       __le32 flags;
>>> +#define SUPPORTS_STATE_SET_NOTIFY(x)   ((x) & BIT(31))
>>> +#define SUPPORTS_STATE_SET_ASYNC(x)    ((x) & BIT(30))
>>> +#define SUPPORTS_STATE_SET_SYNC(x)     ((x) & BIT(29))
>>> +           u8 name[SCMI_MAX_STR_SIZE];
>>> +} __packed;
>>
>> I think it would be better to leave out the __packed here, which
>> can lead to rather inefficient code. It's only really a problem when
>> building with -mstrict-align, but it's better to write code in a way that
>> doesn't rely on that.
>>
>
> I assume you are referring to above structure only and not general
> across all the structures ? I will have a look at this one.

I meant all of them, from my first look they all seem to have natural
alignment on all members anyway. If there is one that doesn't, I would
suggest annotating the individual unaligned members with __packed.

>>> +static int
>>> +scmi_power_domain_attributes_get(struct scmi_handle *handle, u32 domain,
>>> +                                struct power_dom_info *dom_info)
>>> +{
>>> +       int ret;
>>> +       struct scmi_xfer *t;
>>> +       struct scmi_msg_resp_power_domain_attributes *attr;
>>> +
>>> +       ret = scmi_one_xfer_init(handle, POWER_DOMAIN_ATTRIBUTES,
>>> +                                SCMI_PROTOCOL_POWER, sizeof(domain),
>>> +                                sizeof(*attr), &t);
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       *(__le32 *)t->tx.buf = cpu_to_le32(domain);
>>> +       attr = (struct scmi_msg_resp_power_domain_attributes *)t->rx.buf;
>>
>> It seems you require a similar pattern in each caller of scmi_one_xfer_init(),
>> but it seems a little clumsy to always require those casts, so maybe there
>> is a nicer way to do this. How about making scmi_one_xfer_init() act
>> as an allocation function and having it return the buffer or a PTR_ERR?
>>
>
> Yes I agree it doesn't looks all nice. I have changed these few times
> while developing and then thought it's better to get some suggestions. I
> am open to any suggestions that will help to make these nicer.
>
>> It also seems odd to have it named 'init' but actually allocate the scmi_xfer
>> structure rather than filling a local variable that gets passed by reference.
>>
>
> It does initialise but partially. scmi_one_xfer_get does pure allocation
> while scmi_one_xfer_init initialise header variables and also tx/rx
> size. But if you think it's odd, I will looks at ways to make it better.

Yes, I'm still thinking about it, but I think we can do better. If a function
has both allocation and initialization parts in it, I would probably name
it *_alloc() rather than *_init().

What is the relation between scmi_one_xfer_get() and
scmi_one_xfer_init()? Do we need both in some callers, or
just one of the two?

       Arnd

[toc] | [prev] | [next] | [standalone]

#1661063 — Re: [RFC PATCH 6/8] firmware: arm_scmi: add initial support for power protocol

On 08/06/17 12:06, Arnd Bergmann wrote:
> On Thu, Jun 8, 2017 at 11:39 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
>>
>> On 07/06/17 21:38, Arnd Bergmann wrote:
>>> On Wed, Jun 7, 2017 at 6:10 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>
>>>> +struct scmi_msg_resp_power_attributes {
>>>> +       __le16 num_domains;
>>>> +       __le16 reserved;
>>>> +       __le32 stats_addr_low;
>>>> +       __le32 stats_addr_high;
>>>> +       __le32 stats_size;
>>>> +} __packed;
>>>> +
>>>> +struct scmi_msg_resp_power_domain_attributes {
>>>> +       __le32 flags;
>>>> +#define SUPPORTS_STATE_SET_NOTIFY(x)   ((x) & BIT(31))
>>>> +#define SUPPORTS_STATE_SET_ASYNC(x)    ((x) & BIT(30))
>>>> +#define SUPPORTS_STATE_SET_SYNC(x)     ((x) & BIT(29))
>>>> +           u8 name[SCMI_MAX_STR_SIZE];
>>>> +} __packed;
>>>
>>> I think it would be better to leave out the __packed here, which
>>> can lead to rather inefficient code. It's only really a problem when
>>> building with -mstrict-align, but it's better to write code in a way that
>>> doesn't rely on that.
>>>
>>
>> I assume you are referring to above structure only and not general
>> across all the structures ? I will have a look at this one.
> 
> I meant all of them, from my first look they all seem to have natural
> alignment on all members anyway. If there is one that doesn't, I would
> suggest annotating the individual unaligned members with __packed.
> 

OK, I will take a deeper look. Thanks for the suggestion.

>>>> +static int
>>>> +scmi_power_domain_attributes_get(struct scmi_handle *handle, u32 domain,
>>>> +                                struct power_dom_info *dom_info)
>>>> +{
>>>> +       int ret;
>>>> +       struct scmi_xfer *t;
>>>> +       struct scmi_msg_resp_power_domain_attributes *attr;
>>>> +
>>>> +       ret = scmi_one_xfer_init(handle, POWER_DOMAIN_ATTRIBUTES,
>>>> +                                SCMI_PROTOCOL_POWER, sizeof(domain),
>>>> +                                sizeof(*attr), &t);
>>>> +       if (ret)
>>>> +               return ret;
>>>> +
>>>> +       *(__le32 *)t->tx.buf = cpu_to_le32(domain);
>>>> +       attr = (struct scmi_msg_resp_power_domain_attributes *)t->rx.buf;
>>>
>>> It seems you require a similar pattern in each caller of scmi_one_xfer_init(),
>>> but it seems a little clumsy to always require those casts, so maybe there
>>> is a nicer way to do this. How about making scmi_one_xfer_init() act
>>> as an allocation function and having it return the buffer or a PTR_ERR?
>>>
>>
>> Yes I agree it doesn't looks all nice. I have changed these few times
>> while developing and then thought it's better to get some suggestions. I
>> am open to any suggestions that will help to make these nicer.
>>
>>> It also seems odd to have it named 'init' but actually allocate the scmi_xfer
>>> structure rather than filling a local variable that gets passed by reference.
>>>
>>
>> It does initialise but partially. scmi_one_xfer_get does pure allocation
>> while scmi_one_xfer_init initialise header variables and also tx/rx
>> size. But if you think it's odd, I will looks at ways to make it better.
> 
> Yes, I'm still thinking about it, but I think we can do better. If a function
> has both allocation and initialization parts in it, I would probably name
> it *_alloc() rather than *_init().
> 
> What is the relation between scmi_one_xfer_get() and
> scmi_one_xfer_init()? Do we need both in some callers, or
> just one of the two?

Currently only scmi_one_xfer_init is used. Initially I was using
scmi_one_xfer_get and initialising at callsite. Strictly speaking, all
the allocations are done at probe time, it's only grabbing and releasing
one at a time at runtime, hence the name _get and _put. I can merge
_init into _get. The way it-is is just artifact of how it got developed :(
-- 
Regards,
Sudeep

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web