kernel-janitors.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "luobin (L)" <luobin9@huawei.com>
To: kernel-janitors@vger.kernel.org
Subject: Re: [bug report] hinic: add mailbox function support
Date: Sat, 20 Jun 2020 01:26:59 +0000	[thread overview]
Message-ID: <189e5546-0268-6619-d806-d2effcefba0b@huawei.com> (raw)
In-Reply-To: <20200619100327.GA245452@mwanda>

On 2020/6/19 18:03, Dan Carpenter wrote:
> Hello Luo bin,
> 
> The patch a425b6e1c69b: "hinic: add mailbox function support" from
> Apr 25, 2020, leads to the following static checker warning:
> 
> 	drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c:817 hinic_init_hwdev()
> 	error: passing non negative 65280 to ERR_PTR
> 
> drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c
>    716  struct hinic_hwdev *hinic_init_hwdev(struct pci_dev *pdev)
>    717  {
>    718          struct hinic_pfhwdev *pfhwdev;
>    719          struct hinic_hwdev *hwdev;
>    720          struct hinic_hwif *hwif;
>    721          int err, num_aeqs;
>    722  
>    723          hwif = devm_kzalloc(&pdev->dev, sizeof(*hwif), GFP_KERNEL);
>    724          if (!hwif)
>    725                  return ERR_PTR(-ENOMEM);
>    726  
>    727          err = hinic_init_hwif(hwif, pdev);
>    728          if (err) {
>    729                  dev_err(&pdev->dev, "Failed to init HW interface\n");
>    730                  return ERR_PTR(err);
>    731          }
>    732  
>    733          pfhwdev = devm_kzalloc(&pdev->dev, sizeof(*pfhwdev), GFP_KERNEL);
>    734          if (!pfhwdev) {
>    735                  err = -ENOMEM;
>    736                  goto err_pfhwdev_alloc;
>    737          }
>    738  
>    739          hwdev = &pfhwdev->hwdev;
>    740          hwdev->hwif = hwif;
>    741  
>    742          err = init_msix(hwdev);
>    743          if (err) {
>    744                  dev_err(&pdev->dev, "Failed to init msix\n");
>    745                  goto err_init_msix;
>    746          }
>    747  
>    748          err = wait_for_outbound_state(hwdev);
>    749          if (err) {
>    750                  dev_warn(&pdev->dev, "outbound - disabled, try again\n");
>    751                  hinic_outbound_state_set(hwif, HINIC_OUTBOUND_ENABLE);
>    752          }
>    753  
>    754          num_aeqs = HINIC_HWIF_NUM_AEQS(hwif);
>    755  
>    756          err = hinic_aeqs_init(&hwdev->aeqs, hwif, num_aeqs,
>    757                                HINIC_DEFAULT_AEQ_LEN, HINIC_EQ_PAGE_SIZE,
>    758                                hwdev->msix_entries);
>    759          if (err) {
>    760                  dev_err(&pdev->dev, "Failed to init async event queues\n");
>    761                  goto err_aeqs_init;
>    762          }
>    763  
>    764          err = init_pfhwdev(pfhwdev);
>    765          if (err) {
>    766                  dev_err(&pdev->dev, "Failed to init PF HW device\n");
>    767                  goto err_init_pfhwdev;
>    768          }
>    769  
>    770          err = hinic_l2nic_reset(hwdev);
>    771          if (err)
>    772                  goto err_l2nic_reset;
>    773  
>    774          err = get_dev_cap(hwdev);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^
> This function sometimes returns positive values on error which will
> lead to an Oops eventually.
> 
>    775          if (err) {
>    776                  dev_err(&pdev->dev, "Failed to get device capabilities\n");
>    777                  goto err_dev_cap;
>    778          }
>    779  
>    780          err = hinic_vf_func_init(hwdev);
>    781          if (err) {
>    782                  dev_err(&pdev->dev, "Failed to init nic mbox\n");
>    783                  goto err_vf_func_init;
>    784          }
>    785  
>    786          err = init_fw_ctxt(hwdev);
>    787          if (err) {
>    788                  dev_err(&pdev->dev, "Failed to init function table\n");
>    789                  goto err_init_fw_ctxt;
>    790          }
>    791  
>    792          err = set_resources_state(hwdev, HINIC_RES_ACTIVE);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Same here according to Smatch.
> 
>    793          if (err) {
>    794                  dev_err(&pdev->dev, "Failed to set resources state\n");
>    795                  goto err_resources_state;
>    796          }
>    797  
>    798          return hwdev;
>    799  
>    800  err_resources_state:
>    801  err_init_fw_ctxt:
>    802          hinic_vf_func_free(hwdev);
>    803  err_vf_func_init:
>    804  err_l2nic_reset:
>    805  err_dev_cap:
>    806          free_pfhwdev(pfhwdev);
>    807  
>    808  err_init_pfhwdev:
>    809          hinic_aeqs_free(&hwdev->aeqs);
>    810  
>    811  err_aeqs_init:
>    812          disable_msix(hwdev);
>    813  
>    814  err_init_msix:
>    815  err_pfhwdev_alloc:
>    816          hinic_free_hwif(hwif);
>    817          return ERR_PTR(err);
>                                ^^^
> If "err" isn't a negative error code it will lead to an Oops in the
> caller.
> 
>    818  }
> 
> One function which will cause an Oops is mbox_resp_info_handler():
> 
> drivers/net/ethernet/huawei/hinic/hinic_hw_mbox.c
>    832  static int mbox_resp_info_handler(struct hinic_mbox_func_to_func *func_to_func,
>    833                                    struct hinic_recv_mbox *mbox_for_resp,
>    834                                    enum hinic_mod_type mod, u16 cmd,
>    835                                    void *buf_out, u16 *out_size)
>    836  {
>    837          int err;
>    838  
>    839          if (mbox_for_resp->msg_info.status) {
>    840                  err = mbox_for_resp->msg_info.status;
>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
>    841                  if (err != HINIC_MBOX_PF_BUSY_ACTIVE_FW)
>    842                          dev_err(&func_to_func->hwif->pdev->dev, "Mbox response error(0x%x)\n",
>    843                                  mbox_for_resp->msg_info.status);
>    844                  return err;
>                                ^^^
> Here err is a u8 so it can't be a negative error code.
> 
>    845          }
>    846  
>    847          if (buf_out && out_size) {
>    848                  if (*out_size < mbox_for_resp->mbox_len) {
>    849                          dev_err(&func_to_func->hwif->pdev->dev,
>    850                                  "Invalid response mbox message length: %d for mod %d cmd %d, should less than: %d\n",
>    851                                  mbox_for_resp->mbox_len, mod, cmd, *out_size);
>    852                          return -EFAULT;
>    853                  }
>    854  
>    855                  if (mbox_for_resp->mbox_len)
>    856                          memcpy(buf_out, mbox_for_resp->mbox,
>    857                                 mbox_for_resp->mbox_len);
>    858  
>    859                  *out_size = mbox_for_resp->mbox_len;
>    860          }
>    861  
>    862          return 0;
>    863  }
> 
> I guess the main problem is functions like send_mbox_seg() which do:
> 
> drivers/net/ethernet/huawei/hinic/hinic_hw_mbox.c
>    662  static int send_mbox_seg(struct hinic_mbox_func_to_func *func_to_func,
>    663                           u64 header, u16 dst_func, void *seg, u16 seg_len,
>    664                           int poll, void *msg_info)
>    665  {
>    666          struct hinic_send_mbox *send_mbox = &func_to_func->send_mbox;
>    667          u16 seq_dir = HINIC_MBOX_HEADER_GET(header, DIRECTION);
>    668          struct hinic_hwdev *hwdev = func_to_func->hwdev;
>    669          struct completion *done = &send_mbox->send_done;
>    670          u8 num_aeqs = hwdev->hwif->attr.num_aeqs;
>    671          u16 dst_aeqn, wb_status = 0, errcode;
>    672  
>    673          if (num_aeqs >= 4)
>    674                  dst_aeqn = (seq_dir = HINIC_HWIF_DIRECT_SEND) ?
>    675                             HINIC_MBOX_RECV_AEQN : HINIC_MBOX_RSP_AEQN;
>    676          else
>    677                  dst_aeqn = 0;
>    678  
>    679          if (!poll)
>    680                  init_completion(done);
>    681  
>    682          clear_mbox_status(send_mbox);
>    683  
>    684          mbox_copy_header(hwdev, send_mbox, &header);
>    685  
>    686          mbox_copy_send_data(hwdev, send_mbox, seg, seg_len);
>    687  
>    688          write_mbox_msg_attr(func_to_func, dst_func, dst_aeqn, seg_len, poll);
>    689  
>    690          wmb(); /* writing the mbox msg attributes */
>    691  
>    692          if (wait_for_mbox_seg_completion(func_to_func, poll, &wb_status))
>    693                  return -ETIMEDOUT;
>    694  
>    695          if (!MBOX_STATUS_SUCCESS(wb_status)) {
>    696                  dev_err(&hwdev->hwif->pdev->dev, "Send mailbox segment to function %d error, wb status: 0x%x\n",
>    697                          dst_func, wb_status);
>    698                  errcode = MBOX_STATUS_ERRCODE(wb_status);
>    699                  return errcode ? errcode : -EFAULT;
>                                          ^^^^^^^
> Positive error code leads to Oops.
> 
>    700          }
>    701  
>    702          return 0;
>    703  }
> 
> I'm not sure if that's every problem, but it's a start.  ;)
> 
> regards,
> dan carpenter
> .
> 
I'll fix it. Thanks for your review.

      reply	other threads:[~2020-06-20  1:26 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-19 10:03 [bug report] hinic: add mailbox function support Dan Carpenter
2020-06-20  1:26 ` luobin (L) [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=189e5546-0268-6619-d806-d2effcefba0b@huawei.com \
    --to=luobin9@huawei.com \
    --cc=kernel-janitors@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).