Re: [RFC PATCH 00/15] acrn: add the ACRN driver module
On Mon, Aug 19, 2019 at 09:44:25AM +0800, Zhao, Yakui wrote: > Not sure whether it can be sent in two patch sets? > The first is to add the required APIs for ACRN driver. > The second is to add the ACRN driver One patchset adding the APIs and its user(s). And make sure to refresh on https://www.kernel.org/doc/html/latest/process/submitting-patches.html before sending. Thx. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH 00/15] acrn: add the ACRN driver module
On 2019年08月19日 13:25, Greg KH wrote: On Mon, Aug 19, 2019 at 09:44:25AM +0800, Zhao, Yakui wrote: On 2019年08月16日 14:39, Borislav Petkov wrote: On Fri, Aug 16, 2019 at 10:25:41AM +0800, Zhao Yakui wrote: The first three patches are the changes under x86/acrn, which adds the required APIs for the driver and reports the X2APIC caps. The remaining patches add the ACRN driver module, which accepts the ioctl from user-space and then communicate with the low-level ACRN hypervisor by using hypercall. I have a problem with that: you're adding interfaces to arch/x86/ and its users go into staging. Why? Why not directly put the driver where it belongs, clean it up properly and submit it like everything else is submitted? Thanks for your reply and the concern. After taking a look at several driver examples(gma500, android), it seems that they are firstly added into drivers/staging/XXX and then moved to drivers/XXX after the driver becomes mature. So we refer to this method to upstream ACRN driver part. Those two examples are probably the worst examples to ever look at :) The code quality of those submissions was horrible, gma500 took a very long time to clean up and there are parts of the android code that are still in staging to this day. If the new driver can also be added by skipping the staging approach, we will refine it and then submit it in normal process. That is the normal process, staging should not be needed at all for any code. It is a fall-back for when the company involved has no idea of how to upstream their code, which should NOT be the case here. Thanks for your explanation. OK. We will submit it in normal process. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH 00/15] acrn: add the ACRN driver module
On Mon, Aug 19, 2019 at 10:39:32AM +0800, Zhao, Yakui wrote: > > > On 2019年08月16日 15:03, Greg KH wrote: > > On Fri, Aug 16, 2019 at 08:39:25AM +0200, Borislav Petkov wrote: > > > On Fri, Aug 16, 2019 at 10:25:41AM +0800, Zhao Yakui wrote: > > > > The first three patches are the changes under x86/acrn, which adds the > > > > required APIs for the driver and reports the X2APIC caps. > > > > The remaining patches add the ACRN driver module, which accepts the > > > > ioctl > > > > from user-space and then communicate with the low-level ACRN hypervisor > > > > by using hypercall. > > > > > > I have a problem with that: you're adding interfaces to arch/x86/ and > > > its users go into staging. Why? Why not directly put the driver where > > > it belongs, clean it up properly and submit it like everything else is > > > submitted? > > > > > > I don't want to have stuff in arch/x86/ which is used solely by code in > > > staging and the latter is lingering there indefinitely because no one is > > > cleaning it up... > > > > I agree, stuff in drivers/staging/ must be self-contained, with no > > changes outside of the code's subdirectory needed in order for it to > > work. That way it is trivial for us to delete it when it never gets > > cleaned up :) > > Thanks for pointing out the rule of drivers/staging. > The acrn staging driver is one self-contained driver. But it has some > dependency on arch/x86/acrn and need to call the APIs in arch/x86/acrn. Then it should not be in drivers/staging/ Please work to get this accepted "normally". thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH 00/15] acrn: add the ACRN driver module
On Mon, Aug 19, 2019 at 09:44:25AM +0800, Zhao, Yakui wrote: > > > On 2019年08月16日 14:39, Borislav Petkov wrote: > > On Fri, Aug 16, 2019 at 10:25:41AM +0800, Zhao Yakui wrote: > > > The first three patches are the changes under x86/acrn, which adds the > > > required APIs for the driver and reports the X2APIC caps. > > > The remaining patches add the ACRN driver module, which accepts the ioctl > > > from user-space and then communicate with the low-level ACRN hypervisor > > > by using hypercall. > > > > I have a problem with that: you're adding interfaces to arch/x86/ and > > its users go into staging. Why? Why not directly put the driver where > > it belongs, clean it up properly and submit it like everything else is > > submitted? > > Thanks for your reply and the concern. > > After taking a look at several driver examples(gma500, android), it seems > that they are firstly added into drivers/staging/XXX and then moved to > drivers/XXX after the driver becomes mature. > So we refer to this method to upstream ACRN driver part. Those two examples are probably the worst examples to ever look at :) The code quality of those submissions was horrible, gma500 took a very long time to clean up and there are parts of the android code that are still in staging to this day. > If the new driver can also be added by skipping the staging approach, > we will refine it and then submit it in normal process. That is the normal process, staging should not be needed at all for any code. It is a fall-back for when the company involved has no idea of how to upstream their code, which should NOT be the case here. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH 00/15] acrn: add the ACRN driver module
On 2019年08月16日 15:03, Greg KH wrote: On Fri, Aug 16, 2019 at 08:39:25AM +0200, Borislav Petkov wrote: On Fri, Aug 16, 2019 at 10:25:41AM +0800, Zhao Yakui wrote: The first three patches are the changes under x86/acrn, which adds the required APIs for the driver and reports the X2APIC caps. The remaining patches add the ACRN driver module, which accepts the ioctl from user-space and then communicate with the low-level ACRN hypervisor by using hypercall. I have a problem with that: you're adding interfaces to arch/x86/ and its users go into staging. Why? Why not directly put the driver where it belongs, clean it up properly and submit it like everything else is submitted? I don't want to have stuff in arch/x86/ which is used solely by code in staging and the latter is lingering there indefinitely because no one is cleaning it up... I agree, stuff in drivers/staging/ must be self-contained, with no changes outside of the code's subdirectory needed in order for it to work. That way it is trivial for us to delete it when it never gets cleaned up :) Thanks for pointing out the rule of drivers/staging. The acrn staging driver is one self-contained driver. But it has some dependency on arch/x86/acrn and need to call the APIs in arch/x86/acrn. If there is no driver, the API without user had better not be added. If API is not added, the driver can't be compiled correctly. The ACRN driver is one new driver. Maybe it will have some bugs and not be mature. So we want to add the driver as the staging. What is the better approach to handle such scenario? You never say _why_ this should go into drivers/staging/, nor do you have a TODO file like all other staging code that explains exactly what needs to be done to get it out of there. Ok. The TODO file will be added in next version. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH 00/15] acrn: add the ACRN driver module
On 2019年08月16日 14:39, Borislav Petkov wrote: On Fri, Aug 16, 2019 at 10:25:41AM +0800, Zhao Yakui wrote: The first three patches are the changes under x86/acrn, which adds the required APIs for the driver and reports the X2APIC caps. The remaining patches add the ACRN driver module, which accepts the ioctl from user-space and then communicate with the low-level ACRN hypervisor by using hypercall. I have a problem with that: you're adding interfaces to arch/x86/ and its users go into staging. Why? Why not directly put the driver where it belongs, clean it up properly and submit it like everything else is submitted? Thanks for your reply and the concern. After taking a look at several driver examples(gma500, android), it seems that they are firstly added into drivers/staging/XXX and then moved to drivers/XXX after the driver becomes mature. So we refer to this method to upstream ACRN driver part. If the new driver can also be added by skipping the staging approach, we will refine it and then submit it in normal process. I don't want to have stuff in arch/x86/ which is used solely by code in staging and the latter is lingering there indefinitely because no one is cleaning it up... The ACRN driver will be the only user of the added APIs in x86/acrn. Without the APIs in x86/acrn, the driver can't add the driver-specifc upcall notification ISR or call the hypercall. Not sure whether it can be sent in two patch sets? The first is to add the required APIs for ACRN driver. The second is to add the ACRN driver Thanks Yakui ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH 00/15] acrn: add the ACRN driver module
On Fri, Aug 16, 2019 at 08:39:25AM +0200, Borislav Petkov wrote: > On Fri, Aug 16, 2019 at 10:25:41AM +0800, Zhao Yakui wrote: > > The first three patches are the changes under x86/acrn, which adds the > > required APIs for the driver and reports the X2APIC caps. > > The remaining patches add the ACRN driver module, which accepts the ioctl > > from user-space and then communicate with the low-level ACRN hypervisor > > by using hypercall. > > I have a problem with that: you're adding interfaces to arch/x86/ and > its users go into staging. Why? Why not directly put the driver where > it belongs, clean it up properly and submit it like everything else is > submitted? > > I don't want to have stuff in arch/x86/ which is used solely by code in > staging and the latter is lingering there indefinitely because no one is > cleaning it up... I agree, stuff in drivers/staging/ must be self-contained, with no changes outside of the code's subdirectory needed in order for it to work. That way it is trivial for us to delete it when it never gets cleaned up :) You never say _why_ this should go into drivers/staging/, nor do you have a TODO file like all other staging code that explains exactly what needs to be done to get it out of there. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH 00/15] acrn: add the ACRN driver module
On Fri, Aug 16, 2019 at 10:25:41AM +0800, Zhao Yakui wrote: > The first three patches are the changes under x86/acrn, which adds the > required APIs for the driver and reports the X2APIC caps. > The remaining patches add the ACRN driver module, which accepts the ioctl > from user-space and then communicate with the low-level ACRN hypervisor > by using hypercall. I have a problem with that: you're adding interfaces to arch/x86/ and its users go into staging. Why? Why not directly put the driver where it belongs, clean it up properly and submit it like everything else is submitted? I don't want to have stuff in arch/x86/ which is used solely by code in staging and the latter is lingering there indefinitely because no one is cleaning it up... -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
