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: <2746d50a-5658-5058-4369-1a1b34f85710@gmail.com>
Date:   Wed, 6 Sep 2023 16:03:06 +0530
From:   Ayush Singh <ayushdevel1325@...il.com>
To:     Krzysztof Kozlowski <krzk@...nel.org>, greybus-dev@...ts.linaro.org
Cc:     devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        gregkh@...uxfoundation.org,
        Vaishnav M A <vaishnav@...gleboard.org>,
        Jason Kridner <jkridner@...gleboard.org>,
        Nishanth Menon <nm@...com>
Subject: Re: [PATCH v4 2/3] greybus: Add BeaglePlay Linux Driver

On 9/6/23 15:29, Krzysztof Kozlowski wrote:

> On 05/09/2023 18:27, Ayush Singh wrote:
>>>> +static void hdlc_handle_rx_frame(struct gb_beagleplay *bg)
>>>> +{
>>>> +	u8 address = bg->rx_buffer[0];
>>>> +	char *buffer = &bg->rx_buffer[2];
>>>> +	size_t buffer_len = bg->rx_buffer_len - 4;
>>>> +
>>>> +	switch (address) {
>>>> +	case ADDRESS_DBG:
>>>> +		hdlc_handle_dbg_frame(bg, buffer, buffer_len);
>>>> +		break;
>>>> +	case ADDRESS_GREYBUS:
>>>> +		hdlc_handle_greybus_frame(bg, buffer, buffer_len);
>>>> +		break;
>>>> +	default:
>>>> +		dev_warn(&bg->serdev->dev, "Got Unknown Frame %u", address);
>>> ratelimit
>>> Probably as well in several places with possible flooding.
>> I don't think `hdlc_handle_rx_frame` is the correct place since it only
>> processes a single completed HDLC frame.  The more appropriate place
>> would be `hdlc_rx` if we want to limit based on the number of HDLC
>> frames or `gb_beagleplay_tty_receive` to limit based on the number of bytes.
>>
>> I would like to ask, though, why is rate limiting required here? Won't
>> `serdev_device_ops->receive_buf` already rate limit the number of bytes
>> somewhat? Or is it related to blocking in the
>> `serdev_device_ops->receive_buf` callback? In the case of latter, it
>> would probably make sense to ratelimit based on number of frames, I think.
> My comment might not be accurate, so I do not insist. The name of the
> function suggested something being called very often (on every frame),
> thus you would print warning also very often.
>
> Best regards,
> Krzysztof
>
Rate limiting the logs is not a bad idea. Initially I was not aware you 
meant about logging, hence the question. With proper firmware in CC1352, 
the warning will never be printed. But maybe it can cause problem with 
improper firmware.


Ayush Singh

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ