linux-kernel - Re: [PATCH 4/4] mmc: sdhci-brcmstb: rpmb sharing by claiming host for TZOS

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 for Android: free password hash cracker in your pocket

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

Message-ID: <3a297f23-9b80-4623-ad58-85de85a5b8f7@kernel.org>
Date: Sun, 13 Jul 2025 10:44:47 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Kamal Dasu <kamal.dasu@...adcom.com>, andersson@...nel.org,
 baolin.wang@...ux.alibaba.com, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org, florian.fainelli@...adcom.com, ulf.hansson@...aro.org,
 adrian.hunter@...el.com
Cc: bcm-kernel-feedback-list@...adcom.com, linux-remoteproc@...r.kernel.org,
 devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, linux-mmc@...r.kernel.org,
 Kamal Dasu <kdasu@...adcom.com>
Subject: Re: [PATCH 4/4] mmc: sdhci-brcmstb: rpmb sharing by claiming host for
 TZOS

On 11/07/2025 17:42, Kamal Dasu wrote:
> +
> +static int sdhci_brcmstb_sdio_share_init(struct platform_device *pdev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(&pdev->dev);
> +	struct device_node *np = pdev->dev.of_node;
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_brcmstb_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +	struct brcmstb_sdio_share_info *si;
> +	void __iomem *sdio_sh_regs;
> +	int ret;
> +
> +	/* sdio_share block */
> +	sdio_sh_regs = devm_platform_ioremap_resource_byname(pdev, "share");
> +	if (IS_ERR(sdio_sh_regs))
> +		return 0;
> +
> +	si = devm_kcalloc(&pdev->dev, 1, sizeof(struct brcmstb_sdio_share_info),

sizeof(*)

> +			  GFP_KERNEL);
> +	if (!si)
> +		return -ENOMEM;
> +
> +	si->share_reg = sdio_sh_regs;
> +	ret = of_hwspin_lock_get_id(np, 0);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to get hwspinlock id %d\n", ret);

Uh? So you changed the ABI in the driver but not in the bindings? No,
this must be always synced. Look what your binding is saying. Is it
optional? Yes.

And why would you print errors anyway on deferred probe? Twice! One here
and warning in your probe.

> +		return ret;
> +	}
> +
> +	si->hwlock = devm_hwspin_lock_request_specific(&pdev->dev, ret);
> +	if (!si->hwlock) {
> +		dev_err(&pdev->dev, "failed to request hwspinlock\n");

Syntax is: return dev_err_probe

> +		return -ENXIO;
> +	}
> +
> +	si->irq_recv = platform_get_irq_byname_optional(pdev, "recv_ipi0");
> +	if (si->irq_recv < 0) {
> +		ret = si->irq_recv;
> +		dev_err(&pdev->dev, "recv_ipi0 IRQ not found\n");

Syntax is: return dev_err_probe

> +		return ret;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, si->irq_recv,
> +			       sdhci_brcmstb_recv_ipi0_irq,
> +			       0, "mmc_recv_ipi0", host);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "mmc_recv_ipi0 IRQ request_irq failed\n");
> +		return ret;

return dev_err_probe

> +	}
> +
> +	si->ipis0_reg = devm_platform_ioremap_resource_byname(pdev, "flshr_ipis0");
> +	if (IS_ERR(si->ipis0_reg))
> +		return -ENXIO;
> +
> +	priv->si = si;
> +	si->host = host;
> +	init_waitqueue_head(&si->wq);
> +	/* acquire hwsem */
> +	sdhci_brcmstb_aquire_hwsem(si);
> +	si->claim_thread =
> +		kthread_run(sdhci_brcmstb_host_claim_thread, si,
> +			    "ksdshrthread/%s", mmc_hostname(host->mmc));
> +	if (IS_ERR(si->claim_thread)) {
> +		ret = PTR_ERR(si->claim_thread);
> +		dev_err(&pdev->dev, "failed to run claim thread\n");
> +		return -ENOEXEC;
> +	}
> +
> +	return 0;
> +}
> +
>  static u32 sdhci_brcmstb_cqhci_irq(struct sdhci_host *host, u32 intmask)
>  {
>  	int cmd_error = 0;
> @@ -482,8 +720,11 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
>  		goto err;
>  
>  	pltfm_host->clk = clk;
> -	return res;
> +	res  = sdhci_brcmstb_sdio_share_init(pdev);

Only single space before '='

> +	if (res)
> +		dev_warn(&pdev->dev, "sdio share unavailable\n");

Why do you warn on completely optional hwlock? Drop, this should be
silent. You already print errors earlier and there is no point to print
error twice.

>  
> +	return 0;
>  err:
>  	sdhci_pltfm_free(pdev);
>  	clk_disable_unprepare(base_clk);


Best regards,
Krzysztof