netdev - Re: [PATCH v4 3/3] Bluetooth: NXP: Add protocol support for NXP Bluetooth chipsets

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: <Y/T1uMqUeW67tgzX@kroah.com>
Date:   Tue, 21 Feb 2023 17:47:52 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Neeraj Sanjay Kale <neeraj.sanjaykale@....com>
Cc:     davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, marcel@...tmann.org,
        johan.hedberg@...il.com, luiz.dentz@...il.com,
        jirislaby@...nel.org, alok.a.tiwari@...cle.com, hdanton@...a.com,
        ilpo.jarvinen@...ux.intel.com, leon@...nel.org,
        netdev@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-bluetooth@...r.kernel.org,
        linux-serial@...r.kernel.org, amitkumar.karwar@....com,
        rohit.fule@....com, sherry.sun@....com
Subject: Re: [PATCH v4 3/3] Bluetooth: NXP: Add protocol support for NXP
 Bluetooth chipsets

On Tue, Feb 21, 2023 at 09:55:41PM +0530, Neeraj Sanjay Kale wrote:
> +		bt_dev_info(hdev, "Set UART break: %s, status=%d",
> +			    ps_state == PS_STATE_AWAKE ? "off" : "on", status);

You have a lot of "noise" in this driver, remove all "info" messages, as
if a driver is working properly, it is quiet.

> +		break;
> +	}
> +	psdata->ps_state = ps_state;
> +	if (ps_state == PS_STATE_AWAKE)
> +		btnxpuart_tx_wakeup(nxpdev);
> +}
> +
> +static void ps_work_func(struct work_struct *work)
> +{
> +	struct ps_data *data = container_of(work, struct ps_data, work);
> +
> +	if (!data)
> +		return;

You did not test this, that check can never happen, please do not do
pointless checks.



> +
> +	if (data->ps_cmd == PS_CMD_ENTER_PS && data->cur_psmode == PS_MODE_ENABLE)
> +		ps_control(data->hdev, PS_STATE_SLEEP);
> +	else if (data->ps_cmd == PS_CMD_EXIT_PS)
> +		ps_control(data->hdev, PS_STATE_AWAKE);
> +}
> +
> +static void ps_timeout_func(struct timer_list *t)
> +{
> +	struct ps_data *data = from_timer(data, t, ps_timer);
> +	struct hci_dev *hdev = data->hdev;
> +	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +
> +	data->timer_on = false;
> +	if (test_bit(BTNXPUART_TX_STATE_ACTIVE, &nxpdev->tx_state)) {
> +		ps_start_timer(nxpdev);
> +	} else {
> +		data->ps_cmd = PS_CMD_ENTER_PS;
> +		schedule_work(&data->work);
> +	}
> +}
> +
> +static int ps_init_work(struct hci_dev *hdev)
> +{
> +	struct ps_data *psdata = kzalloc(sizeof(*psdata), GFP_KERNEL);

Don't do allocations in variable declarations :(

> +	} else if (req_len == sizeof(uart_config)) {
> +		uart_config.clkdiv.address = __cpu_to_le32(CLKDIVADDR);
> +		uart_config.clkdiv.value = __cpu_to_le32(0x00c00000);
> +		uart_config.uartdiv.address = __cpu_to_le32(UARTDIVADDR);
> +		uart_config.uartdiv.value = __cpu_to_le32(1);
> +		uart_config.mcr.address = __cpu_to_le32(UARTMCRADDR);
> +		uart_config.mcr.value = __cpu_to_le32(MCR);
> +		uart_config.re_init.address = __cpu_to_le32(UARTREINITADDR);
> +		uart_config.re_init.value = __cpu_to_le32(INIT);
> +		uart_config.icr.address = __cpu_to_le32(UARTICRADDR);
> +		uart_config.icr.value = __cpu_to_le32(ICR);
> +		uart_config.fcr.address = __cpu_to_le32(UARTFCRADDR);
> +		uart_config.fcr.value = __cpu_to_le32(FCR);
> +		uart_config.crc = swab32(nxp_fw_dnld_update_crc(0UL,
> +								(char *)&uart_config,
> +								sizeof(uart_config) - 4));
> +		serdev_device_write_buf(nxpdev->serdev, (u8 *)&uart_config, req_len);
> +		serdev_device_wait_until_sent(nxpdev->serdev, 0);

You are sending magic commands over the serial connection, are you sure
that is ok?

> +	if (requested_len & 0x01) {
> +		/* The CRC did not match at the other end.
> +		 * Simply send the same bytes again.
> +		 */
> +		requested_len = nxpdev->fw_v1_sent_bytes;
> +		bt_dev_err(hdev, "CRC error. Resend %d bytes of FW.", requested_len);

Why is this an error sent to the kernel log?

Again, be quiet if there is nothing that a user can do.

thanks,

greg k-h