On Mon, Jul 31, 2017 at 09:40:41PM +0300, Alexander Bezzubikov wrote:
+typedef struct PCIEPCIBridge {
- /*< private >*/
- PCIBridge parent_obj;
- bool msi_enable;
Please rename the msi_enable property to "msi" in order to be aligned with the existent PCIBridgeDev and consider making it OnOffAuto for the same reason. (I am not sure about the last part though, we have no meaning for "auto" here)
Agreed about "msi", but OnOffAuto looks weird to me as we always want MSI to be enabled.
Why even have a property then? Can't you enable it unconditionally?
On 01/08/2017 18:32, Michael S. Tsirkin wrote:
On Mon, Jul 31, 2017 at 09:40:41PM +0300, Alexander Bezzubikov wrote:
+typedef struct PCIEPCIBridge {
- /*< private >*/
- PCIBridge parent_obj;
- bool msi_enable;
Please rename the msi_enable property to "msi" in order to be aligned with the existent PCIBridgeDev and consider making it OnOffAuto for the same reason. (I am not sure about the last part though, we have no meaning for "auto" here)
Agreed about "msi", but OnOffAuto looks weird to me as we always want MSI to be enabled.
Hi Michael,
Why even have a property then? Can't you enable it unconditionally?
Because of a current bug in Linux kernel: https://www.spinics.net/lists/linux-pci/msg63052.html msi will not work until the patch is merged. Even when it will be merged, not all linux kernels will contain the patch.
Disabling msi is a workaround for the above case.
Thanks, Marcel
On Tue, Aug 01, 2017 at 06:45:13PM +0300, Marcel Apfelbaum wrote:
On 01/08/2017 18:32, Michael S. Tsirkin wrote:
On Mon, Jul 31, 2017 at 09:40:41PM +0300, Alexander Bezzubikov wrote:
+typedef struct PCIEPCIBridge {
- /*< private >*/
- PCIBridge parent_obj;
- bool msi_enable;
Please rename the msi_enable property to "msi" in order to be aligned with the existent PCIBridgeDev and consider making it OnOffAuto for the same reason. (I am not sure about the last part though, we have no meaning for "auto" here)
Agreed about "msi", but OnOffAuto looks weird to me as we always want MSI to be enabled.
Hi Michael,
Why even have a property then? Can't you enable it unconditionally?
Because of a current bug in Linux kernel: https://www.spinics.net/lists/linux-pci/msg63052.html msi will not work until the patch is merged. Even when it will be merged, not all linux kernels will contain the patch.
You should Cc stable to make sure they all gain it eventually.
Disabling msi is a workaround for the above case.
Thanks, Marcel
Really enabling MSI without bus master is a bug that I'm not 100% sure it even worth working around. But I guess it's not too bad to have a work-around given it's this simple.
On 01/08/2017 18:51, Michael S. Tsirkin wrote:
On Tue, Aug 01, 2017 at 06:45:13PM +0300, Marcel Apfelbaum wrote:
On 01/08/2017 18:32, Michael S. Tsirkin wrote:
On Mon, Jul 31, 2017 at 09:40:41PM +0300, Alexander Bezzubikov wrote:
+typedef struct PCIEPCIBridge {
- /*< private >*/
- PCIBridge parent_obj;
- bool msi_enable;
Please rename the msi_enable property to "msi" in order to be aligned with the existent PCIBridgeDev and consider making it OnOffAuto for the same reason. (I am not sure about the last part though, we have no meaning for "auto" here)
Agreed about "msi", but OnOffAuto looks weird to me as we always want MSI to be enabled.
Hi Michael,
Why even have a property then? Can't you enable it unconditionally?
Because of a current bug in Linux kernel: https://www.spinics.net/lists/linux-pci/msg63052.html msi will not work until the patch is merged. Even when it will be merged, not all linux kernels will contain the patch.
You should Cc stable to make sure they all gain it eventually.
Right! thanks, we missed cc-ing stable. Added stable to the mail thread. Marcel
Disabling msi is a workaround for the above case.
Thanks, Marcel
Really enabling MSI without bus master is a bug that I'm not 100% sure it even worth working around. But I guess it's not too bad to have a work-around given it's this simple.
-
Marcel Apfelbaum -
Michael S. Tsirkin