linux-kernel - Re: [PATCH 2/3] fsi: occ: Add support for P10

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <df7cee27-ef84-d846-7b7b-1d5fdd4a5f25@roeck-us.net>
Date:   Mon, 20 Jul 2020 07:36:59 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Joel Stanley <joel@....id.au>
Cc:     Eddie James <eajames@...ux.ibm.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-hwmon@...r.kernel.org, linux-fsi@...ts.ozlabs.org,
        devicetree <devicetree@...r.kernel.org>,
        Jean Delvare <jdelvare@...e.com>,
        Alistair Popple <alistair@...ple.id.au>,
        Jeremy Kerr <jk@...abs.org>, Rob Herring <robh+dt@...nel.org>
Subject: Re: [PATCH 2/3] fsi: occ: Add support for P10

On 7/19/20 9:47 PM, Joel Stanley wrote:
> On Sun, 19 Jul 2020 at 22:13, Guenter Roeck <linux@...ck-us.net> wrote:
>>
>> On Fri, May 01, 2020 at 10:08:32AM -0500, Eddie James wrote:
>>> The P10 OCC has a different SRAM address for the command and response
>>> buffers. In addition, the SBE commands to access the SRAM have changed
>>> format. Add versioning to the driver to handle these differences.
>>>
>>> Signed-off-by: Eddie James <eajames@...ux.ibm.com>
>>
>> I don't recall seeing a maintainer Ack for this patch, nor any response
>> at all. I'd be happy to apply the patch through hwmon, but I would need
>> an Ack from a maintainer.
> 
> That would be great. I had one question before it goes in, but once
> Eddie has sorted that out it can go through your tree.
> 
>>
>> Thanks,
>> Guenter
>>
>>> ---
>>>  drivers/fsi/fsi-occ.c | 126 ++++++++++++++++++++++++++++++------------
>>>  1 file changed, 92 insertions(+), 34 deletions(-)
> 
>>> @@ -508,6 +557,7 @@ static int occ_probe(struct platform_device *pdev)
>>>       struct occ *occ;
>>>       struct platform_device *hwmon_dev;
>>>       struct device *dev = &pdev->dev;
>>> +     const void *md =  of_device_get_match_data(dev);
>>>       struct platform_device_info hwmon_dev_info = {
>>>               .parent = dev,
>>>               .name = "occ-hwmon",
>>> @@ -517,6 +567,7 @@ static int occ_probe(struct platform_device *pdev)
>>>       if (!occ)
>>>               return -ENOMEM;
>>>
>>> +     occ->version = (enum versions)md;
> 
> The 0day bot warns about this when bulding for 64 bit architectures.
> 
> How about you drop the match data and instead match on the compatible
> string to know which version to expect?
> 

That seems to be less desirable and defeat the purpose of of_device_get_match_data().
I have seen better solutions. Some options:

	version = (uintptr_t)of_device_get_match_data(dev);
	version = (unsigned long)of_device_get_match_data(dev);
	version = (enum versions)(uintptr_t)of_device_get_match_data(dev);

You don't otherwise use the "md" variable, so you might as well drop it.

Guenter

>>>       occ->dev = dev;
>>>       occ->sbefifo = dev->parent;
>>>       mutex_init(&occ->occ_lock);
>>> @@ -575,7 +626,14 @@ static int occ_remove(struct platform_device *pdev)
>>>  }
>>>
>>>  static const struct of_device_id occ_match[] = {
>>> -     { .compatible = "ibm,p9-occ" },
>>> +     {
>>> +             .compatible = "ibm,p9-occ",
>>> +             .data = (void *)occ_p9
>>> +     },
>>> +     {
>>> +             .compatible = "ibm,p10-occ",
>>> +             .data = (void *)occ_p10
>>> +     },
>>>       { },
>>>  };
>>>