| 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: <3c067b26-cfe8-4939-afce-5c8753767715@wanadoo.fr>
Date: Sat, 14 Dec 2024 16:52:35 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Markuss Broks <markuss.broks@...il.com>
Cc: linux-samsung-soc@...r.kernel.org, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
Ivaylo Ivanov <ivo.ivanov.ivanov1@...il.com>,
Markuss Broks <markuss.broks@...il.com>,
Maksym Holovach <nergzd@...gzd723.xyz>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Alim Akhtar <alim.akhtar@...sung.com>,
Krzysztof Kozlowski <krzk@...nel.org>
Subject: Re: [PATCH 2/3] soc: samsung: Add a driver for Samsung SPEEDY host
controller
Le 12/12/2024 à 22:09, Markuss Broks a écrit :
> Add a driver for Samsung SPEEDY serial bus host controller.
> SPEEDY is a proprietary 1 wire serial bus used by Samsung
> in various devices (usually mobile), like Samsung Galaxy
> phones. It is usually used for connecting PMIC or various
> other peripherals, like audio codecs or RF components.
>
> This bus can address at most 1MiB (4 bit device address,
> 8 bit registers per device, 8 bit wide registers:
> 256*256*16 = 1MiB of address space.
...
> +static int _speedy_read(struct speedy_controller *speedy, u32 reg, u32 addr, u32 *val)
> +{
> + int ret;
> + u32 cmd, int_ctl, int_status;
> +
> + mutex_lock(&speedy->io_lock);
All error handling paths fail to release the mutex.
guard(mutex) would help here.
> +
> + ret = speedy_fifo_reset(speedy);
> + if (ret)
> + return ret;
> +
> + ret = regmap_set_bits(speedy->map, SPEEDY_FIFO_CTRL,
> + SPEEDY_RX_LENGTH(1) | SPEEDY_TX_LENGTH(1));
> + if (ret)
> + return ret;
> +
> + cmd = SPEEDY_ACCESS_RANDOM | SPEEDY_DIRECTION_READ |
> + SPEEDY_DEVICE(reg) | SPEEDY_ADDRESS(addr);
> +
> + int_ctl = SPEEDY_TRANSFER_DONE_EN | SPEEDY_FIFO_RX_ALMOST_FULL_EN |
> + SPEEDY_RX_FIFO_INT_TRAILER_EN | SPEEDY_RX_MODEBIT_ERR_EN |
> + SPEEDY_RX_GLITCH_ERR_EN | SPEEDY_RX_ENDBIT_ERR_EN |
> + SPEEDY_REMOTE_RESET_REQ_EN;
> +
> + ret = speedy_int_clear(speedy);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(speedy->map, SPEEDY_INT_ENABLE, int_ctl);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(speedy->map, SPEEDY_CMD, cmd);
> + if (ret)
> + return ret;
> +
> + /* Wait for xfer done */
> + ret = regmap_read_poll_timeout(speedy->map, SPEEDY_INT_STATUS, int_status,
> + int_status & SPEEDY_TRANSFER_DONE, 5000, 50000);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(speedy->map, SPEEDY_RX_DATA, val);
> + if (ret)
> + return ret;
> +
> + ret = speedy_int_clear(speedy);
> +
> + mutex_unlock(&speedy->io_lock);
> +
> + return ret;
> +}
...
> +static int _speedy_write(struct speedy_controller *speedy, u32 reg, u32 addr, u32 val)
> +{
> + int ret;
> + u32 cmd, int_ctl, int_status;
> +
> + mutex_lock(&speedy->io_lock);
> +
> + ret = speedy_fifo_reset(speedy);
> + if (ret)
> + return ret;
All error handling paths fail to release the mutex.
guard(mutex) would help here.
> +
> + ret = regmap_set_bits(speedy->map, SPEEDY_FIFO_CTRL,
> + SPEEDY_RX_LENGTH(1) | SPEEDY_TX_LENGTH(1));
> + if (ret)
> + return ret;
> +
> + cmd = SPEEDY_ACCESS_RANDOM | SPEEDY_DIRECTION_WRITE |
> + SPEEDY_DEVICE(reg) | SPEEDY_ADDRESS(addr);
> +
> + int_ctl = (SPEEDY_TRANSFER_DONE_EN |
> + SPEEDY_FIFO_TX_ALMOST_EMPTY_EN |
> + SPEEDY_TX_LINE_BUSY_ERR_EN |
> + SPEEDY_TX_STOPBIT_ERR_EN |
> + SPEEDY_REMOTE_RESET_REQ_EN);
> +
> + ret = speedy_int_clear(speedy);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(speedy->map, SPEEDY_INT_ENABLE, int_ctl);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(speedy->map, SPEEDY_CMD, cmd);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(speedy->map, SPEEDY_TX_DATA, val);
> + if (ret)
> + return ret;
> +
> + /* Wait for xfer done */
> + ret = regmap_read_poll_timeout(speedy->map, SPEEDY_INT_STATUS, int_status,
> + int_status & SPEEDY_TRANSFER_DONE, 5000, 50000);
> + if (ret)
> + return ret;
> +
> + speedy_int_clear(speedy);
> +
> + mutex_unlock(&speedy->io_lock);
> +
> + return 0;
> +}
...
> +/**
> + * speedy_get_by_phandle() - internal get speedy device handle
> + * @np: pointer to OF device node of device
> + *
> + * Return: 0 on success, -errno otherwise
On success, a handle is returned, not 0.
> + */
> +static const struct speedy_device *speedy_get_device(struct device_node *np)
> +{
...
> +out:
> + of_node_put(speedy_np);
> + return handle;
> +}
...
> +static int speedy_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct speedy_controller *speedy;
> + void __iomem *mem;
> + int ret;
> +
> + speedy = devm_kzalloc(dev, sizeof(struct speedy_controller), GFP_KERNEL);
> + if (!speedy)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, speedy);
> + speedy->pdev = pdev;
> +
> + mutex_init(&speedy->io_lock);
> +
> + mem = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(mem))
> + return dev_err_probe(dev, PTR_ERR(mem), "Failed to ioremap memory\n");
> +
> + speedy->map = devm_regmap_init_mmio(dev, mem, &speedy_map_cfg);
> + if (IS_ERR(speedy->map))
> + return dev_err_probe(dev, PTR_ERR(speedy->map), "Failed to init the regmap\n");
> +
> + /* Clear any interrupt status remaining */
> + ret = speedy_int_clear(speedy);
> + if (ret)
> + return ret;
> +
> + /* Reset the controller */
> + ret = regmap_set_bits(speedy->map, SPEEDY_CTRL, SPEEDY_SW_RST);
> + if (ret)
> + return ret;
> +
> + msleep(20);
> +
> + /* Enable the hw */
> + ret = regmap_set_bits(speedy->map, SPEEDY_CTRL, SPEEDY_ENABLE);
> + if (ret)
> + return ret;
> +
> + msleep(20);
> +
> + /* Probe child devices */
> + ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, dev);
> + if (ret)
> + dev_err(dev, "Failed to populate child devices: %d\n", ret);
Could be dev_err_probe() as well, at least for consistency.
> +
> + return ret;
> +}
...
CJ
Powered by blists - more mailing lists