On Wed, May 17, 2023 at 11:08:12AM +0800, Liu, Jingqi wrote:
/** All drivers support IOMMU_HWPT_TYPE_DEFAULT, so pass it through.* For any other hwpt_type, check the ops->domain_alloc_user_data_len* presence and its result.*/if (cmd->hwpt_type != IOMMU_HWPT_TYPE_DEFAULT) {if (!ops->domain_alloc_user_data_len) {rc = -EOPNOTSUPP;goto out_put_idev;}klen = ops->domain_alloc_user_data_len(cmd->hwpt_type);if (WARN_ON(klen < 0)) {rc = -EINVAL;goto out_put_pt;}Would it be better if the later check "klen" is moved here ? if (klen) { [...] } If this check fails here, there's no need to execute the code after it. If this path is not executed, "klen" is 0, and there's no need to check it. Do I understand it right ?
Makes sense. And the klen value isn't really being used. So, we may likely change it to a bool one. Also, I'm thinking of forcing a !!cmd->data_len for a non-DEFAULT hwpt_type:
+ if (cmd->hwpt_type != IOMMU_HWPT_TYPE_DEFAULT) { + if (!cmd->data_len) { + rc = -EINVAL; + goto out_put_pt; + } + if (!ops->domain_alloc_user_data_len) { + rc = -EOPNOTSUPP; + goto out_put_pt; + } + if (!ops->hwpt_type_is_supported(cmd->hwpt_type)) { + rc = -EINVAL; + goto out_put_pt; + }
Then, for the latter part, simply: + if (cmd->data_len) { + // malloc data + }
Thanks Nic