linux-kernel - Re: [PATCH V2 01/10] accel/amdxdna: Add a new driver for AMD AI Engine

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: <9f017342-b686-4ba0-bb0a-6912b051c152@quicinc.com>
Date: Fri, 9 Aug 2024 08:24:50 -0700
From: Carl Vanderlip <quic_carlv@...cinc.com>
To: Lizhi Hou <lizhi.hou@....com>, <ogabbay@...nel.org>,
        <dri-devel@...ts.freedesktop.org>
CC: <linux-kernel@...r.kernel.org>, <min.ma@....com>, <max.zhen@....com>,
        <sonal.santan@....com>, <king.tam@....com>,
        Narendra Gutta
	<VenkataNarendraKumar.Gutta@....com>,
        George Yang <George.Yang@....com>
Subject: Re: [PATCH V2 01/10] accel/amdxdna: Add a new driver for AMD AI
 Engine

On 8/5/2024 10:39 AM, Lizhi Hou wrote:
 > +static int aie2_init(struct amdxdna_dev *xdna)
 > +{
 > +    struct pci_dev *pdev = to_pci_dev(xdna->ddev.dev);
 > +    struct amdxdna_dev_hdl *ndev;
 > +    struct psp_config psp_conf;
 > +    const struct firmware *fw;
 > +    void __iomem * const *tbl;
 > +    int i, bars, nvec, ret;
 > +
 > +    ndev = devm_kzalloc(&pdev->dev, sizeof(*ndev), GFP_KERNEL);
 > +    if (!ndev)
 > +        return -ENOMEM;
 > +
 > +    ndev->priv = xdna->dev_info->dev_priv;
 > +    ndev->xdna = xdna;
 > +
 > +    ret = request_firmware(&fw, ndev->priv->fw_path, &pdev->dev);
 > +    if (ret) {
 > +        XDNA_ERR(xdna, "failed to request_firmware %s, ret %d",
 > +             ndev->priv->fw_path, ret);
 > +        return ret;
 > +    }
 > +
 > +    ret = pcim_enable_device(pdev);


Does request_firmware need to be the first thing here? Its not used 
until the end of init. Likewise, fw image is copied in *_create, but 
then not released until after *_hw_start; could release_firmware more 
closely wrap where it is used? I could see it being checked first 
because if the fw isn't there, what's the point, but that could be said 
about any of the other resources here.

On 8/5/2024 10:39 AM, Lizhi Hou wrote:
 > +enum aie2_smu_reg_idx {
 > +    SMU_CMD_REG = 0,
 > +    SMU_ARG_REG,
 > +    SMU_INTR_REG,
 > +    SMU_RESP_REG,
 > +    SMU_OUT_REG,
 > +    SMU_MAX_REGS /* Kepp this at the end */
 > +};

*Keep


-Carl V.

PS Sorry for double email Lizhi, forgot to send to list.