Hi,
On 12/12/23 11:36, Suma Hegde wrote:
> Cache pci_dev obj during probe as part of struct hsmp_socket
> and use in amd_hsmp_rdwr(). This change will make it easier to
> support both non-ACPI and ACPI devices.
>
> Also add a check for sock_index agsint number of sockets
> in the hsmp_send_message().
>
> Signed-off-by: Suma Hegde <suma.hegde@xxxxxxx>
> Co-developed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@xxxxxxx>
> Signed-off-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@xxxxxxx>
> ---
> drivers/platform/x86/amd/hsmp.c | 43 +++++++++++++++++++--------------
> 1 file changed, 25 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
> index 3c17b488f4f8..62a274c84f25 100644
> --- a/drivers/platform/x86/amd/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp.c
> @@ -58,6 +58,7 @@ struct hsmp_socket {
> void __iomem *metric_tbl_addr;
> struct semaphore hsmp_sem;
> char name[HSMP_ATTR_GRP_NAME_SIZE];
> + struct pci_dev *root;
> u16 sock_ind;
> };
>
> @@ -71,17 +72,20 @@ struct hsmp_plat_device {
>
> static struct hsmp_plat_device plat_dev;
>
> -static int amd_hsmp_rdwr(struct pci_dev *root, u32 address,
> +static int amd_hsmp_rdwr(struct hsmp_socket *sock, u32 address,
> u32 *value, bool write)
> {
> int ret;
>
> - ret = pci_write_config_dword(root, HSMP_INDEX_REG, address);
> + if (!sock->root)
> + return -ENODEV;
> +
> + ret = pci_write_config_dword(sock->root, HSMP_INDEX_REG, address);
> if (ret)
> return ret;
>
> - ret = (write ? pci_write_config_dword(root, HSMP_DATA_REG, *value)
> - : pci_read_config_dword(root, HSMP_DATA_REG, value));
> + ret = (write ? pci_write_config_dword(sock->root, HSMP_DATA_REG, *value)
> + : pci_read_config_dword(sock->root, HSMP_DATA_REG, value));
>
> return ret;
> }
> @@ -95,7 +99,7 @@ static int amd_hsmp_rdwr(struct pci_dev *root, u32 address,
> * Returns 0 for success and populates the requested number of arguments.
> * Returns a negative error code for failure.
> */
> -static int __hsmp_send_message(struct pci_dev *root, struct hsmp_message *msg)
> +static int __hsmp_send_message(struct hsmp_socket *sock, struct hsmp_message *msg)
> {
> unsigned long timeout, short_sleep;
> u32 mbox_status;
> @@ -104,7 +108,7 @@ static int __hsmp_send_message(struct pci_dev *root, struct hsmp_message *msg)
>
> /* Clear the status register */
> mbox_status = HSMP_STATUS_NOT_READY;
> - ret = amd_hsmp_rdwr(root, SMN_HSMP_MSG_RESP, &mbox_status, HSMP_WR);
> + ret = amd_hsmp_rdwr(sock, SMN_HSMP_MSG_RESP, &mbox_status, HSMP_WR);
> if (ret) {
> pr_err("Error %d clearing mailbox status register\n", ret);
> return ret;
> @@ -113,7 +117,7 @@ static int __hsmp_send_message(struct pci_dev *root, struct hsmp_message *msg)
> index = 0;
> /* Write any message arguments */
> while (index < msg->num_args) {
> - ret = amd_hsmp_rdwr(root, SMN_HSMP_MSG_DATA + (index << 2),
> + ret = amd_hsmp_rdwr(sock, SMN_HSMP_MSG_DATA + (index << 2),
> &msg->args[index], HSMP_WR);
> if (ret) {
> pr_err("Error %d writing message argument %d\n", ret, index);
> @@ -123,7 +127,7 @@ static int __hsmp_send_message(struct pci_dev *root, struct hsmp_message *msg)
> }
>
> /* Write the message ID which starts the operation */
> - ret = amd_hsmp_rdwr(root, SMN_HSMP_MSG_ID, &msg->msg_id, HSMP_WR);
> + ret = amd_hsmp_rdwr(sock, SMN_HSMP_MSG_ID, &msg->msg_id, HSMP_WR);
> if (ret) {
> pr_err("Error %d writing message ID %u\n", ret, msg->msg_id);
> return ret;
> @@ -140,7 +144,7 @@ static int __hsmp_send_message(struct pci_dev *root, struct hsmp_message *msg)
> timeout = jiffies + msecs_to_jiffies(HSMP_MSG_TIMEOUT);
>
> while (time_before(jiffies, timeout)) {
> - ret = amd_hsmp_rdwr(root, SMN_HSMP_MSG_RESP, &mbox_status, HSMP_RD);
> + ret = amd_hsmp_rdwr(sock, SMN_HSMP_MSG_RESP, &mbox_status, HSMP_RD);
> if (ret) {
> pr_err("Error %d reading mailbox status\n", ret);
> return ret;
> @@ -175,7 +179,7 @@ static int __hsmp_send_message(struct pci_dev *root, struct hsmp_message *msg)
> */
> index = 0;
> while (index < msg->response_sz) {
> - ret = amd_hsmp_rdwr(root, SMN_HSMP_MSG_DATA + (index << 2),
> + ret = amd_hsmp_rdwr(sock, SMN_HSMP_MSG_DATA + (index << 2),
> &msg->args[index], HSMP_RD);
> if (ret) {
> pr_err("Error %d reading response %u for message ID:%u\n",
> @@ -208,21 +212,20 @@ static int validate_message(struct hsmp_message *msg)
>
> int hsmp_send_message(struct hsmp_message *msg)
> {
> - struct hsmp_socket *sock = &plat_dev.sock[msg->sock_ind];
> - struct amd_northbridge *nb;
> + struct hsmp_socket *sock;
> int ret;
>
> if (!msg)
> return -EINVAL;
> -
> - nb = node_to_amd_nb(msg->sock_ind);
> - if (!nb || !nb->root)
> - return -ENODEV;
> -
> ret = validate_message(msg);
> if (ret)
> return ret;
>
> + if (!plat_dev.sock || msg->sock_ind >= plat_dev.num_sockets)
> + return -ENODEV;
> + sock = &plat_dev.sock[msg->sock_ind];
> + if (!sock)
> + return -ENODEV;
You have already checked that plat_dev.sock != NULL so sock
will always be non NULL here and this check is redundant.
Please drop the "if (!sock)" check.
Otherwise this looks good so with that fixed this is:
Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>
Regards,
Hans
> /*
> * The time taken by smu operation to complete is between
> * 10us to 1ms. Sometime it may take more time.
> @@ -233,7 +236,7 @@ int hsmp_send_message(struct hsmp_message *msg)
> if (ret < 0)
> return ret;
>
> - ret = __hsmp_send_message(nb->root, msg);
> + ret = __hsmp_send_message(sock, msg);
>
> up(&sock->hsmp_sem);
>
> @@ -500,6 +503,10 @@ static int hsmp_pltdrv_probe(struct platform_device *pdev)
> sema_init(&plat_dev.sock[i].hsmp_sem, 1);
> plat_dev.sock[i].sock_ind = i;
>
> + if (!node_to_amd_nb(i))
> + return -ENODEV;
> + plat_dev.sock[i].root = node_to_amd_nb(i)->root;
> +
> /* Test the hsmp interface on each socket */
> ret = hsmp_test(i, 0xDEADBEEF);
> if (ret) {
