* [bug report] hinic: add mailbox function support
@ 2020-06-19 10:03 Dan Carpenter
2020-06-20 1:26 ` luobin (L)
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2020-06-19 10:03 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [bug report] hinic: add mailbox function support
2020-06-19 10:03 [bug report] hinic: add mailbox function support Dan Carpenter
@ 2020-06-20 1:26 ` luobin (L)
0 siblings, 0 replies; 2+ messages in thread
From: luobin (L) @ 2020-06-20 1:26 UTC (permalink / raw)
To: kernel-janitors
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.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-06-20 1:26 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-19 10:03 [bug report] hinic: add mailbox function support Dan Carpenter
2020-06-20 1:26 ` luobin (L)
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).