| 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
| ||
|
Message-ID: <b52ea6f3-a547-ebce-88f5-6256501a7e99@linux.intel.com>
Date: Wed, 24 Jul 2019 11:59:12 -0500
From: Thor Thayer <thor.thayer@...ux.intel.com>
To: Moritz Fischer <mdf@...nel.org>
Cc: richard.gong@...ux.intel.com, agust@...x.de,
linux-fpga@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCHv2 2/3] fpga: altera-cvp: Preparation for V2 parts.
Hi Moritz,
On 7/24/19 9:57 AM, Moritz Fischer wrote:
> On Tue, Jul 23, 2019 at 09:40:51AM -0500, Thor Thayer wrote:
>> Hi Moritz,
>>
>> On 7/21/19 7:59 PM, Moritz Fischer wrote:
>>> Thor,
>>>
>>> On Tue, Jul 16, 2019 at 05:48:06PM -0500, thor.thayer@...ux.intel.com wrote:
>>>> From: Thor Thayer <thor.thayer@...ux.intel.com>
>>>>
>>>> In preparation for adding newer V2 parts that use a FIFO,
>>>> reorganize altera_cvp_chk_error() and change the write
>>>> function to block based.
>>>> V2 parts have a block size matching the FIFO while older
>>>> V1 parts write a 32 bit word at a time.
>>>>
>>>> Signed-off-by: Thor Thayer <thor.thayer@...ux.intel.com>
>>>> ---
>>>> v2 Remove inline function declaration
>>>> Reverse Christmas Tree format for local variables
>>>> ---
>>>> drivers/fpga/altera-cvp.c | 72 ++++++++++++++++++++++++++++++-----------------
>>>> 1 file changed, 46 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
>>>> index b78c90580071..37419d6b9915 100644
>>>> --- a/drivers/fpga/altera-cvp.c
>>>> +++ b/drivers/fpga/altera-cvp.c
>>>> @@ -140,6 +140,41 @@ static int altera_cvp_wait_status(struct altera_cvp_conf *conf, u32 status_mask,
>>>> return -ETIMEDOUT;
>>>> }
>>>> +static int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes)
>>>> +{
>>>> + struct altera_cvp_conf *conf = mgr->priv;
>>>> + u32 val;
>>>> +
>>>> + /* STEP 10 (optional) - check CVP_CONFIG_ERROR flag */
>>>> + altera_read_config_dword(conf, VSE_CVP_STATUS, &val);
>>> Same as in the other email, why can we ignore return values here. I
>>> think the original code probably did that already.
>>
>> Yes, I actually didn't make any changes to this function. You can see I
>> moved it from below since it is used in the following function.
>>
>> I'm not checking the return code from any of the read/write functions since
>> the original driver didn't. Would you prefer I check and issue a warning?
>
> Not sure a warning would change much here. We should probably look at
> why it was ok in the first place.
A quick grep of the drivers directory shows that an overwhelming
majority of pci_read_config_dword() and pci_write_config_dword() calls
do not check the return code.
For robustness, I agree with you that checking and returning the return
code in this error checking function is important. I will return the
error code if the read fails.
It shouldn't be necessary to change the rest of the code though unless
you feel strongly about updating the existing codebase.
>>
>> Thanks for reviewing!
>>
>>>> + if (val & VSE_CVP_STATUS_CFG_ERR) {
>>>> + dev_err(&mgr->dev, "CVP_CONFIG_ERROR after %zu bytes!\n",
>>>> + bytes);
>>>> + return -EPROTO;
>>>> + }
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int altera_cvp_send_block(struct altera_cvp_conf *conf,
>>>> + const u32 *data, size_t len)
>>>> +{
>>>> + u32 mask, words = len / sizeof(u32);
>>>> + int i, remainder;
>>>> +
>>>> + for (i = 0; i < words; i++)
>>>> + conf->write_data(conf, *data++);
>>>> +
>>>> + /* write up to 3 trailing bytes, if any */
>>>> + remainder = len % sizeof(u32);
>>>> + if (remainder) {
>>>> + mask = BIT(remainder * 8) - 1;
>>>> + if (mask)
>>>> + conf->write_data(conf, *data & mask);
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> static int altera_cvp_teardown(struct fpga_manager *mgr,
>>>> struct fpga_image_info *info)
>>>> {
>>>> @@ -262,39 +297,29 @@ static int altera_cvp_write_init(struct fpga_manager *mgr,
>>>> return 0;
>>>> }
>>>> -static inline int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes)
>>>> -{
>>>> - struct altera_cvp_conf *conf = mgr->priv;
>>>> - u32 val;
>>>> -
>>>> - /* STEP 10 (optional) - check CVP_CONFIG_ERROR flag */
>>>> - altera_read_config_dword(conf, VSE_CVP_STATUS, &val);
>>>> - if (val & VSE_CVP_STATUS_CFG_ERR) {
>>>> - dev_err(&mgr->dev, "CVP_CONFIG_ERROR after %zu bytes!\n",
>>>> - bytes);
>>>> - return -EPROTO;
>>>> - }
>>>> - return 0;
>>>> -}
>>>> -
>>>> static int altera_cvp_write(struct fpga_manager *mgr, const char *buf,
>>>> size_t count)
>>>> {
>>>> struct altera_cvp_conf *conf = mgr->priv;
>>>> + size_t done, remaining, len;
>>>> const u32 *data;
>>>> - size_t done, remaining;
>>>> int status = 0;
>>>> - u32 mask;
>>>> /* STEP 9 - write 32-bit data from RBF file to CVP data register */
>>>> data = (u32 *)buf;
>>>> remaining = count;
>>>> done = 0;
>>>> - while (remaining >= 4) {
>>>> - conf->write_data(conf, *data++);
>>>> - done += 4;
>>>> - remaining -= 4;
>>>> + while (remaining) {
>>>> + if (remaining >= sizeof(u32))
>>>> + len = sizeof(u32);
>>>> + else
>>>> + len = remaining;
>>>> +
>>>> + altera_cvp_send_block(conf, data, len);
>>>> + data++;
>>>> + done += len;
>>>> + remaining -= len;
>>>> /*
>>>> * STEP 10 (optional) and STEP 11
>>>> @@ -312,11 +337,6 @@ static int altera_cvp_write(struct fpga_manager *mgr, const char *buf,
>>>> }
>>>> }
>>>> - /* write up to 3 trailing bytes, if any */
>>>> - mask = BIT(remaining * 8) - 1;
>>>> - if (mask)
>>>> - conf->write_data(conf, *data & mask);
>>>> -
>>>> if (altera_cvp_chkcfg)
>>>> status = altera_cvp_chk_error(mgr, count);
>>>> --
>>>> 2.7.4
>>>>
>>> Cheers,
>>> Moritz
>>>
>>
>
> Moritz
>
Thor
Powered by blists - more mailing lists