On 8/21/2023 11:18 AM, Saurabh Singh Sengar wrote:
>
>
>> -----Original Message-----
>> From: Nuno Das Neves <nunodasneves@xxxxxxxxxxxxxxxxxxx>
>> Sent: Friday, August 18, 2023 3:32 AM
>> To: linux-hyperv@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>> x86@xxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
>> arch@xxxxxxxxxxxxxxx
>> Cc: patches@xxxxxxxxxxxxxxx; Michael Kelley (LINUX)
>> <mikelley@xxxxxxxxxxxxx>; KY Srinivasan <kys@xxxxxxxxxxxxx>;
>> wei.liu@xxxxxxxxxx; Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>; Dexuan Cui
>> <decui@xxxxxxxxxxxxx>; apais@xxxxxxxxxxxxxxxxxxx; Tianyu Lan
>> <Tianyu.Lan@xxxxxxxxxxxxx>; ssengar@xxxxxxxxxxxxxxxxxxx; MUKESH
>> RATHOR <mukeshrathor@xxxxxxxxxxxxx>; stanislav.kinsburskiy@xxxxxxxxx;
>> jinankjain@xxxxxxxxxxxxxxxxxxx; vkuznets <vkuznets@xxxxxxxxxx>;
>> tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; bp@xxxxxxxxx;
>> dave.hansen@xxxxxxxxxxxxxxx; hpa@xxxxxxxxx; will@xxxxxxxxxx;
>> catalin.marinas@xxxxxxx
>> Subject: [PATCH v2 15/15] Drivers: hv: Add modules to expose /dev/mshv to
>> VMMs running on Hyper-V
>>
>> Add mshv, mshv_root, and mshv_vtl modules:
>>
>
> <snip>
>
>> + ret = mshv_set_vp_registers(vp->index, vp->partition->id,
>> + 1, &dispatch_suspend);
>> + if (ret)
>> + pr_err("%s: failed to suspend partition %llu vp %u\n",
>> + __func__, vp->partition->id, vp->index);
>> +
>> + return ret;
>> +}
>> +
>> +static int
>> +get_vp_signaled_count(struct mshv_vp *vp, u64 *count)
>> +{
>> + int ret;
>> + struct hv_register_assoc root_signal_count = {
>> + .name = HV_REGISTER_VP_ROOT_SIGNAL_COUNT,
>> + };
>> +
>> + ret = mshv_get_vp_registers(vp->index, vp->partition->id,
>> + 1, &root_signal_count);
>> +
>> + if (ret) {
>> + pr_err("%s: failed to get root signal count for partition %llu vp
>> %u",
>> + __func__, vp->partition->id, vp->index);
>> + *count = 0;
>
> Have we missed a return here ?
> Moreover, the return type of this function is never checked consider
> checking it or change it to void.
>
Thanks, we do need to return here.
This function is called on a cleanup path (deleting the guest VM), so if it fails
something is wrong. Instead of returning void, I think we should check the return
value with WARN_ON(), and abort the cleanup if it failed.
>> +
>> +/* Retrieve and stash the supported scheduler type */
>> +static int __init mshv_retrieve_scheduler_type(void)
>> +{
>> + struct hv_input_get_system_property *input;
>> + struct hv_output_get_system_property *output;
>> + unsigned long flags;
>> + u64 status;
>> +
>> + local_irq_save(flags);
>> + input = *this_cpu_ptr(hyperv_pcpu_input_arg);
>> + output = *this_cpu_ptr(hyperv_pcpu_output_arg);
>> +
>> + memset(input, 0, sizeof(*input));
>> + memset(output, 0, sizeof(*output));
>> + input->property_id = HV_SYSTEM_PROPERTY_SCHEDULER_TYPE;
>> +
>> + status = hv_do_hypercall(HVCALL_GET_SYSTEM_PROPERTY, input,
>> output);
>> + if (!hv_result_success(status)) {
>> + local_irq_restore(flags);
>> + pr_err("%s: %s\n", __func__, hv_status_to_string(status));
>> + return hv_status_to_errno(status);
>> + }
>> +
>> + hv_scheduler_type = output->scheduler_type;
>> + local_irq_restore(flags);
>> +
>> + pr_info("mshv: hypervisor using %s\n",
>> scheduler_type_to_string(hv_scheduler_type));
>
> Nit: In this file we are using two styles of prints, few are appended with
> "mshv:" and few with "__func__". It's better to have a single style
> for one module.
Thanks, I'll switch them all to __func__
