linux-kernel - Re: [PATCH v3 03/16] fpga: mgr: API change to replace fpga load functions with single function

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CANk1AXQpHfrrFWNixj601KuMzrBuwC_Xd=GPaiyMtpD_umxcuQ@mail.gmail.com>
Date:   Mon, 10 Jul 2017 13:23:08 -0500
From:   Alan Tull <atull@...nel.org>
To:     Moritz Fischer <moritz.fischer@...us.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-fpga@...r.kernel.org
Subject: Re: [PATCH v3 03/16] fpga: mgr: API change to replace fpga load
 functions with single function

On Mon, Jul 10, 2017 at 12:36 PM, Moritz Fischer
<moritz.fischer@...us.com> wrote:

Hi Moritz,

Thanks for looking at this stuff.

>>
>> +struct fpga_image_info *fpga_image_info_alloc(struct device *dev)
>> +{
>> +       struct fpga_image_info *info;
>> +
>> +       info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
>> +       if (!info)
>> +               return ERR_PTR(-ENOMEM);
>
> Doesn't this make it more complex?

Yes!

> If in the end you'll anyway have to check
> if IS_ERR_OR_NULL()? As opposed to just checking if (!info) on the returned
> value.
>
> Just a thought.

Yes, I agree.

>> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
>> index 655940f..c71c9cb 100644
>> --- a/drivers/fpga/fpga-region.c
>> +++ b/drivers/fpga/fpga-region.c
>> @@ -226,14 +226,11 @@ static int fpga_region_get_bridges(struct fpga_region *region,
>>  /**
>>   * fpga_region_program_fpga - program FPGA
>>   * @region: FPGA region
>> - * @firmware_name: name of FPGA image firmware file
>>   * @overlay: device node of the overlay
>> - * Program an FPGA using information in the device tree.
>> - * Function assumes that there is a firmware-name property.
>> + * Program an FPGA using information in the region's fpga image info.
>>   * Return 0 for success or negative error code.
>>   */
>>  static int fpga_region_program_fpga(struct fpga_region *region,
>> -                                   const char *firmware_name,
>>                                     struct device_node *overlay)
>>  {
>>         struct fpga_manager *mgr;
>> @@ -264,7 +261,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
>>                 goto err_put_br;
>>         }
>>
>> -       ret = fpga_mgr_firmware_load(mgr, region->info, firmware_name);
>> +       ret = fpga_mgr_load(mgr, region->info);
>>         if (ret) {
>>                 pr_err("failed to load fpga image\n");
>>                 goto err_put_br;
>> @@ -357,16 +354,15 @@ static int child_regions_with_firmware(struct device_node *overlay)
>>  static int fpga_region_notify_pre_apply(struct fpga_region *region,
>>                                         struct of_overlay_notify_data *nd)
>>  {
>> -       const char *firmware_name = NULL;
>> +       struct device *dev = &region->dev;
>>         struct fpga_image_info *info;
>> +       const char *firmware_name;
>>         int ret;
>>
>> -       info = devm_kzalloc(&region->dev, sizeof(*info), GFP_KERNEL);
>> +       info = fpga_image_info_alloc(dev);
>>         if (!info)
>>                 return -ENOMEM;
>
> Can't you return PTR_ERR(info) here (If you don't follow my suggestion above),
> or keep it the same here and follow my suggestion ;-)

I see I fell exactly into the trap :) I set for myself.

I'll fix the return value of fpga_image_info_alloc() as you suggested above.

Alan