Hi, Huacai > 2026年8月4日 22:33，Huacai Chen <[email protected]> 写道： > > On Tue, Aug 4, 2026 at 12:09 AM Miao Wang <[email protected]> wrote: >> >> Hi, >> >>> 2026年8月3日 21:45，Huacai Chen <[email protected]> 写道： >>> >>> On Fri, Jul 31, 2026 at 4:24 PM Miao Wang <[email protected]> wrote: >>>> >>>> Hi, >>>> >>>>> 2026年7月31日 16:07，Huacai Chen <[email protected]> 写道： >>>>> >>>>> On Fri, Jul 24, 2026 at 5:28 PM Miao Wang <[email protected]> >>>>> wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>>> 2026年7月24日 16:55，Huacai Chen <[email protected]> 写道： >>>>>>> >>>>>>> Hi, Miao, >>>>>>> >>>>>>> On Fri, Jul 10, 2026 at 1:24 AM Miao Wang via B4 Relay >>>>>>> <[email protected]> wrote: >>>>>>>> >>>>>>>> Previously, the driver has been introduced to support the Loongson 2K >>>>>>>> BMC running on the Loongson Servers, which is essential to prevent >>>>>>>> the system from hanging when the BMC is being reset and the default >>>>>>>> efi-framebuffer is being used. However, there are some drawbacks in the >>>>>>>> driver. >>>>>>>> >>>>>>>> Firstly, the driver tries to read and write to the connected PCI-E host >>>>>>>> controller registers, assuming that the BMC is connected to LS7A PCI-E >>>>>>>> host controller. This assumption should be true for real products, but >>>>>>>> to prevent from accidentally reading and writing to the wrong PCI-E >>>>>>>> host >>>>>>>> controller, this driver should be modified to check this before >>>>>>>> accessing the registers. >>>>>>>> >>>>>>>> Secondly, the driver uses non-exported functions to tell the vt >>>>>>>> subsystem to redraw the screen, preventing the driver from being >>>>>>>> compiling as a module. This can be fixed by using the exported >>>>>>>> functions instead. >>>>>>> You can replace the redraw function, but I don't think it is necessary >>>>>>> to make the bmc driver modular. >>>>>>> >>>>>>> BMC core, IPMI and simpledrm display are usually (if not always) >>>>>>> supposed to work as early as possible. >>>>>> >>>>>> I believe that it should be the user's decision to choose whether to >>>>>> compile a module into the kernel or as a module and it would be better >>>>>> if we can provide the possibilities for the user to choose from. >>>>>> Additionally, I don't think these modules are supposed to work that >>>>>> early. The mfd module provide two functions, the display and the ipmi >>>>>> device. In the aspect of graphical display, without this module, the >>>>>> user can still see the output during booting on their monitors, since >>>>>> efifb is working, providing a basic display function. In the aspect of >>>>>> the ipmi device, I don't think the lack of such device will influence >>>>>> the boot of the system, since it is a common practice to compile ipmi >>>>>> device drivers as modules on other architectures. As a result, neither >>>>>> of the two functions are required to be loaded that early and it is >>>>>> reasonable to at lease leave the choice to compile it as a module >>>>>> to the user. >>>>> Flexibility is not always useful, if a config doesn't provide good >>>>> effect, then it just increases complexity and makes maintenance more >>>>> difficult. >>>> >>>> I should emphasize that to allow this driver to be a module, there is >>>> no such increase on maintenance. Moreover, not all loongarch machines >>>> are requiring this driver. Especially only a part of the server models >>>> are quipped with this BMC. Comparing with other architectures, the >>>> driver for BMC are normally compiled as a module, such as mgag200 for >>>> iLO from HPE and iDRAC from DELL, hibmc_drm for Kunpeng server from >>>> Huawei. None of these BMC drivers requiring to be compiled into the >>>> kernel. I cannot see there is any reason keeping the driver from being >>>> allowed to be compiled as a module. I also do not think it will bring >>>> any significant maintenance burden. Implementing correct cleanup code >>>> should be necessary instead of burden. >>> Can we split into two series, one fix bugs and the others make bmc modular? >>> >>> Otherwise I don't think we can reach a consensus in the near future. >> >> I accept different opinions on design trade-offs. However, I don't think >> you have provided enough excuses to remain this driver as built-in, >> since I believe normally in kernel, most non-core drivers are all able >> to be compiled as a module. I also provided some examples from devices >> with similar functions. As a result, I'll not split this series before >> there is indeed a strong reason against allowing this driver to be >> compiled as a module or we may have a great benefit if we force this >> module to be compiled built-in. > At least you should remove the first patch from this series. Binbin > Zhou has been fixing it: > https://lore.kernel.org/loongarch/[email protected]/ > https://lore.kernel.org/loongarch/[email protected]/ > https://lore.kernel.org/loongarch/[email protected]/
Sorry for not noticing Binbin's patch which had already been addressing the same issue. However, I believe the fix in my series has covered more issues than Binbin's patch and thus should be better in the following aspects: 1. Since the length of the resolution string is short enough, there is no need to allocate buffer from the heap. The buffer can be allocated directly on the stack. The most possible resolution string might be "video=4294967295x4294967295-4294967295@4294967295M\0", which is 51 bytes, and allocating 64 bytes on the stack would be enough. 2. My patch enhances the validation of the mode string. strsep(&ptr, sep) may set ptr to NULL if sep is not found, which will cause later strsep() invocations returning NULL, which is not expected by kstrtoxx() functions. 3. My patch further guards string manipulation functions by adding the ending '\0' to the end of the string buffer, to prevent strsep() from overrunning the end of the buffer. 4. My patch also addresses the type of the data. The fields in the struct simplefb_platform_data are unsigned and thus kstrtouint() should be used instead. If the BMC device is under the control of an attacker somehow and the attacker is feeding malicious resolution string to the host kernel, then the second and the third issue will cause memory issues in the host kernel, which might be dangerous. I wonder if Binbin is willing to address previous issues in that patch. If Binbin is willing to do so, then I will be pleased to drop the first patch from my series. If Binbin agrees, that patch can also be absorbed and replaced by the first patch in my series. Cheers, Miao Wang _______________________________________________ Openipmi-developer mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openipmi-developer
